search

palantir / conjure-java

Conjure generator for Java clients and servers
Apache License 2.0
27 stars 43 forks source link

Track removing JsonIgnoreProperties from beans #206

Open dansanduleac opened 5 years ago

dansanduleac commented 5 years ago

What happened?

Due to a regression introduced earlier on, we generate @JsonIgnoreProperties on all beans:

https://github.com/palantir/conjure-java/blob/0ffbc20056e5296b3af0a32fd86e061b624cecaa/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java#L99-L100

which overrides the distinct configuration we had intended for servers as opposed to clients:

https://github.com/palantir/conjure-java-runtime/blob/f5c8e7e56f874b47b6707a7ce214f7a36dfdc2d8/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java#L67

What did you want to happen?

https://github.com/palantir/conjure-java/pull/121 should be merged once we are certain that this won't break existing clients.

a-k-g commented 4 years ago

Another thing to note is that there's a slight inconsistency in the ignoreUnknown - we only set that on beans that have at least one field. Beans that are empty don't have it. So, if a server defines an empty Response object and later adds fields to it, it may break certain clients that are using the old definitions. Clients who use the Conjure ObjectMappers.newClientObjectMapper will not be affected.

gatesn commented 4 years ago

Can I bump this? It's pretty seriously wrong that my incorrect request returned a 200 response when it didn't actually do anything.

In my specific case, the bean had a single Set<> field. I typo'd the name of the field and I got a 200 response, presumably because the server saw an empty set and figured it had no work to do.