-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add serialization order tests (JavaBean style) #3932
Conversation
@yihtserns this test result does not seem to match what you say in #3925 . When the JsonProperty annotation only appears on the constructor param, it appears to be ignored. It does not affect the order and it does not even affect the name of the field in the JSON. |
If what @pjfanning is true, can we revise what was said in the mentioned comment? just in case of confusing anybody |
You're missing public record NestedRecordOne(
String id,
String email,
@JsonProperty(value = "yikes!") NestedRecordTwo nestedRecordTwo) {
} ...is not equivalent to this: public class NestedClassOne {
private String id;
private String name;
@JsonProperty(value = "all is fine!") // 👈 not just here
private NestedClassTwo nestedClassTwo;
public NestedClassOne(String id,
String name,
NestedClassTwo nestedClassTwo) {
this.id = id;
this.name = name;
this.nestedClassTwo = nestedClassTwo;
}
// getters
} ...but is actually equivalent to this: public class NestedClassOne {
private String id;
private String name;
@JsonProperty(value = "all is fine!") // 👈 here
private NestedClassTwo nestedClassTwo;
public NestedClassOne(String id,
String name,
// 👇 and also here
@JsonProperty(value = "all is fine!") NestedClassTwo nestedClassTwo) {
this.id = id;
this.name = name;
this.nestedClassTwo = nestedClassTwo;
}
...
@JsonProperty(value = "all is fine!") // 👈 and also here, but this part is not important
public NestedClassTwo getNestedClassTwo() {
return this.nestedClassTwo;
}
} |
@yihtserns why would you want to put the same JsonProperty annotation in 2 places? |
I wouldn't - I'm just describing the resulting Record's compiled |
Thanks @yihtserns - I updated the PR to have 2 separate classes to highlight the 2 separate issues we are seeing |
@pjfanning I think I could proceed with this, but it needs merge/rebase from 2.16 I think. |
@cowtowncoder the tests in this PR just reproduce a problem. I have not attempted a fix. |
@pjfanning Gotcha. It could be added under |
@pjfanning Is this still useful? If yes, would need to be re-based to 2.18 I think. If not, should close. |
@pjfanning After re-basing to 2.18, updating, tests actually pass now. If you think these are valuable, would be happy to merge? |
@cowtowncoder The tests are still broken. I have moved them to a new class under the failing package. |
@pjfanning But of course. I somehow missed that basic fact. |
Hmmh. No, I don't think tests are quite right here, for couple of reasons. First, without Second: Jackson does sort Creator-bound properties ahead of other properties (unless Third: handling of Records does actually differ from other POJOs, because of ability to auto-detect "canonical constructor". So this probably differs from #3925 no matter what (i.e. expected behavior is not quite identical) There is still one open question here: creator as defined here is invalid for deserialization, with only one of parameters named (and no parameter name detection): it would fail deserialization. For now, I think this is probably more of a feature. But I don't think these tests are valid, as presented. |
See #3925
There is #3928 for records. This PR focuses on JavaBean style classes.
This highlights 2 problems: