fix property case-handling during CRD gen #194
Conversation
|
You're fixing this in our fork of fabric8, but isn't this an issue in Immutables not agreeing with Jackson? |
|
An alternate and completely reasonable solution would be an ErrorProne check that prevents these |
Our fork is a perfectly reasonable place to fix this issue. While it's not something I plan to push upstream, this doesn't just cover cases with Immutables. The internal deserialization in Fabric8 is done with Jackson, and any time the internal properties of a POJO fall into these casing edge cases, Jackson will fail to recognize the field match on deserialization.
This would cause a pretty large disruption to users, who currently have this sort of naming in their Immutables, but it's otherwise working just fine because they are not using generated CRD yaml yet. Names in interfaces would have to change, and existing objects in the API would have to be migrated. I don't think that's a tenable solution. |
|
I don't think it's a big deal to change these objects. the hardest part is having a period where both versions of the serialized form are accepted. i don't think this is a very common use-case. But as you like, if you prefer to fix it in our branch, sure. |
| TypeRef typeRef = schemaFrom != null ? schemaFrom : parameterMap.exchange(original.getTypeRef()); | ||
| String finalName = renamedTo != null ? renamedTo : original.getName(); | ||
|
|
||
| return new Property(original.getAnnotations(), typeRef, finalName, | ||
| original.getComments(), false, false, original.getModifiers(), original.getAttributes()); | ||
| } | ||
|
|
||
| private String getCaseCorrectedPropertyName() { |
There was a problem hiding this comment.
Can we add a comment here describing what "case correction" in this case means?
| @@ -517,7 +517,7 @@ public void process() { | |||
| break; | |||
| case ANNOTATION_JSON_PROPERTY: | |||
| final String nameFromAnnotation = (String) a.getParameters().get(VALUE); | |||
| if (!Strings.isNullOrEmpty(nameFromAnnotation) && !propertyName.equals(nameFromAnnotation)) { | |||
There was a problem hiding this comment.
To test my understanding - this change is necessary because we want the nameFromAnnotation to override anything we might do from getCaseCorrectedPropertyName() ? Since if renamedTo is not null, the result of getCaseCorrectedPropertyName() is unused.
There was a problem hiding this comment.
Yes, exactly. If someone actually wanted getHBaseImage() to generate a CRD field of hBaseImage instead of hbaseImage, then they should use JsonProperty to annotate that getter to override our default conversion, but if the internal field name was already hBaseImage (because maybe the field is generated with Immutables) then without this line setting that wouldn't be detected as an override and we'd use the new convention anyway.
|
Makes sense to me, aside from a clarifying question |
We've noticed that Immutables doesn't follow the same case-conventions as Jackson in cases where the getter is something like
getHBaseImage(). In that case, Immutables makes the field on the POJOhBaseImageand therefore that is the field name in the generated CRDs. Jackson expects the field name in that case to behbaseImage, and therefore there's a deserialization error thrown with the field is required on the POJO.This PR makes sure the field names follow the Jackson case-conventions unless overridden by the
JsonPropertyannotation.