Conversation
|
Guys, when are you planning to merge this PR? |
|
|
||
| private void checkForExplicitFieldOverrides(JsonSchema schema) { | ||
| var schemaNode = schema.getSchemaNode(); | ||
| var propertiesNode = schemaNode.get("definitions"); |
There was a problem hiding this comment.
shouldn't the propertiesNode be under "properties", instead of "definitions"? Please do not use magic constants, make them actual constants
|
|
||
| log.debug("Getting all fields for class: {}", clazz.getName()); | ||
|
|
||
| // Traverse the class hierarchy |
There was a problem hiding this comment.
You should add a test for this. I'm pretty sure that this wont give you the fieldnames used in the json-schema, since the json property names are defined via Jackson-Annotation, rather than the raw-property names.
| existingProperties.addAll(getAllFieldNames(beanDescription.getBeanClass())); | ||
|
|
||
| // Add standard TMForum properties that should never be overridden by schemas | ||
| existingProperties.add("id"); |
There was a problem hiding this comment.
Not every extendable entity has an id or href defined. With this, none such entity could be extended with an id or href.
wistefan
left a comment
There was a problem hiding this comment.
Please make this functionality configurable. It should be deactivated by default and just activated by configuration, since its a quite expensive solution for such an edge case.
| String className = voClass.getName(); | ||
| String baseClassName = null; | ||
|
|
||
| if (className.endsWith("CreateVO")) { |
There was a problem hiding this comment.
If you want to check if something is overwritten by a schema, check the class, not some other connected class. We will not hardcode semantic into the class-names.
Greetings @wistefan
I got a ticket because apparently it is possible to change base fields in TMForum using @schemalocation, I dont think this is intended since in the documentation @schemalocation is described for adding additional attributes rather than changing existing ones. Although I might be wrong...
In case it is not intented, here is a PR that tries to solve that problem by checking with beanDescription the fields in the target class as well as the commons
Here is a curl that can trigger this substitution:
Best regards