search

FasterXML / jackson-module-jsonSchema

Module for generating JSON Schema (v3) definitions from POJOs
371 stars 135 forks source link

Adding support for JSR-303 @NotNull annotation #97

Closed desliner closed 8 years ago

cowtowncoder commented 8 years ago

Looks good to me. One concern I have is that since ValidationConstraintResolver is an interface, adding a new method will be backwards-incompatbile, so this addition could break existing code. This is why I prefer use of abstract classes, in general, before we can make use of default implementations from Java 8.

desliner commented 8 years ago

So you suggest we have an abstract class, e.g. ValidationConstraintResolverSupport implements ValidationConstraintResolver with additional getRequired() method and then using this ValidationConstraintResolverSupport in ValidationSchemaFactoryWrapper instead of ValidationConstraintResolver? If we go for backward compatibility, we should also keep ValidationSchemaFactoryWrapper(ValidationConstraintResolver) constructor. Hence we should also have DelegatingValidationConstraintResolverSupport which delegates interface methods and stubs extension methods to be used in that constructor. Will it be the right approach?

Or we could just have getRequired() method in ValidationSchemaFactoryWrapper directly.

cowtowncoder commented 8 years ago

@mmyslyvtsev Given that it is an interface already, I am not sure how much we can do -- it's more lamentation of not having defined it as an abstract class originally. But then again this whole package is a mess...

So I think addition is acceptable, but change needs to go in 2.8 and can not be made for 2.7 patch release.

I do not remember how delegation model works to comment on possible alternate placement of getRequired(); I assume you considered alternatives and think this makes most sense.

desliner commented 8 years ago

If we are doing a breaking change anyway, maybe it makes sense to change ValidationConstraintResolver from interface to abstract class in 2.8 as well?

Client migration impact will be the same, but we would at least clean up tech debt allowing better maintainability in the future.

cowtowncoder commented 8 years ago

@mmyslyvtsev I went ahead and did something bit similar; added ValidationConstraintResolver.Base as empty implementation. Come to think of that it may or may not be any better than just changing the type, given that any existing implementation need to be changed anyway.

desliner commented 8 years ago

@cowtowncoder Yea, I also think if we are doing a breaking change already - it's better to make it right. ValidationConstraintResolver.Base adds unnecessary complexity. I would prefer to change ValidationConstraintResolver to abstract class while we are at it.

cowtowncoder commented 8 years ago

Done. Decided to leave existing methods (and new one) abstract; anything added at a later point should have default implementation.

desliner commented 8 years ago

@cowtowncoder Looks good to me.

Thanks for quick turnaround with this. When should we expect 2.8 released with these changes available?

cowtowncoder commented 8 years ago

@mmyslyvtsev may take a while, as I have been busy trying to plug bugs in 2.7 so far. I don't have timeline clear yet, but would guess 2 months until release candidates.