-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Remove @JsonTypeName from Java POJOs #14842
base: master
Are you sure you want to change the base?
Remove @JsonTypeName from Java POJOs #14842
Conversation
I created another PR, #14852 which combines this change with the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final ObjectMapper objectMapper = new ObjectMapper(); | ||
|
||
@Test | ||
public void shouldSerializeAndDeserializeModel() throws JsonProcessingException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
*/ | ||
@Test | ||
public void testSpecialCatAllOf() { | ||
// TODO: test SpecialCatAllOf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't plan to. It was created by the generator. It looks like most of the generated tests don't have any implementations in them. Is there anything that you think I should do with it?
import static org.hamcrest.CoreMatchers.is; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
|
||
public class SpecialCatJacksonTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is in the apache-httpclient - probably fine, but the tests here are not apache client specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'm still learning my way around the samples... I moved it into the java/native
sample project. Is that the best place for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think java/native
makes sense since it impacts all of Java
…serialization" This reverts commit 509bc37.
Some other changes that I'm working on affect the behaviour of
@JsonTypeName
annotations in the Java pojo models. After doing some investigation, I concluded that those annotations are unnecessary (confirmed by this stackoverflow answer).@JsonTypeName
is unnecessary when logical type names are specified within@JsonSubTypes
, as is the case in the code generated from this project. As you can see from the generated samples,@JsonTypeName
is also being added to classes that aren't a subclass of another model class, and@JsonTypeName
is pointless in these cases as far as I know.I created this PR as a separate change to limit the scope of changes. It's already large because of the number of samples affected. Removing the import of JsonTypeName is coming in separate PRs, as is removing the annotation from the Spring template.
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10)