Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ea 3545 spike json schema validation #37

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

nshweta90
Copy link

No description provided.

-->

<dependency>
<groupId>com.networknt</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency, shouldn't this be included in api-commons?




private ObjectMapper objectMapper = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure to declare fields on the top

private ObjectMapper objectMapper = new ObjectMapper();

@Autowired
private ResourcePatternResolver resourcePatternResolver;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure to declare fields on the top

@@ -33,7 +35,7 @@ public TranslationController(TranslationWebService translationService) {
@Tag(description = "Translation", name = "translate")
@PostMapping(value = {"/translate"},
produces = {HttpHeaders.CONTENT_TYPE_JSON_UTF8, MediaType.APPLICATION_JSON_VALUE})
public ResponseEntity<String> translate(@RequestBody TranslationRequest translRequest,
public ResponseEntity<String> translate(@ValidJson(uri = TranslationSchemaLocation.JSON_SCHEMA_URI, nested = TranslationSchemaLocation.JSON_SCHEMA_NESTED) TranslationRequest translRequest,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need two nested json schema one for detect and one for translate. here we should use the translate one right?


import eu.europeana.api.commons.JsonSchemaLocation;

public interface TranslationSchemaLocation extends JsonSchemaLocation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class contains our RND server location. These values should be added to the config file and accessed from there.

You can have the nested properties here.
But the only problem is if in the future we update or change the name, you need to make code changes and make a new release. If we add all this in the config file, we can easily redeploy. Better to add this in the config file, and access them.

I think we already have the TranslationConfig class where you can access them and use them in controller

}


@ExceptionHandler(JsonValidationFailedException.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handler for the JsonValidationFailedException class should be added in EuropeanaGlobalExceptionHandler in api-commons.
This way this can be handled by any API using JSON schema validation.

private ResourcePatternResolver resourcePatternResolver;

@Override
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment why are we doing this. something like this -

/**

  • we need to tell Spring about our JsonSchemaValidatingArgumentResolver.
  • We do this by using the addArgumentResolvers(..) method from the WebMvcConfigurer interface.
  • @param resolvers
    */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants