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
fix: @SchemaSwap can only be used once #4354
Conversation
crd-generator/api/src/test/java/io/fabric8/crd/generator/v1/JsonSchemaTest.java
Show resolved
Hide resolved
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.
@xRodney Thanks a lot for this PR!
I really like the overall changes that you did 🙂 , I added a few notes to be discussed.
A few additional requests, since you are fixing a bug can you add test cases for the relevant situations where the current implementation is failing (what you already described in the issue):
SchemaSwap
should be applied to all occurrences in the hierarchySchemaSwap
should be applied to deeply nested structures (e.g. 3 or more levels down)SchemaSwap
should fail if the field name is not matched
@@ -239,11 +256,9 @@ private T internalFromImpl(TypeDef definition, Set<String> visited, List<Interna | |||
continue; | |||
} | |||
|
|||
final PropertyFacade facade = new PropertyFacade(property, accessors, currentSchemaSwaps); | |||
ClassRef potentialSchemaSwap = schemaSwaps.lookupAndMark(definition.toReference(), name).orElse(null); |
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.
If I correctly follow this change, here you cannot mark just yet the SchemaSwap as used, as it will mark the annotation to be "used" even in this condition:
- Matched
Class
- The Class doesn't have the specified
Property
(e.g. a matching field)
This operation should be performed only after process
has been called on the PropertyFacade
(and the SchemaSwap
has been matched).
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.
schemaSwapt.lookupAndMark accepts two arguments, the second being the property name. (Line 253: String name = property.getName();
)
So I'm not sure I understand your concern?
Btw, I thought the case of "matched class, unmatched field" is being tested in JsonSchemaTest.shouldThrowIfSchemaSwapHasUnmatchedField. This test is still passing and I only extended it with assert on the error message. But the error is still thrown.
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.
Or is your concern that the name
could be overriden by @JsonProperty
& friends? IMHO it makes more sense for the fieldName
parameter to reference the fields as is defined in Java, but that's just my opinion.
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.
it makes more sense for the fieldName parameter to reference the fields as is defined in Java, but that's just my opinion.
This makes sense, but, using name
might not be the name extracted by the PropertyFacade
constructor:
kubernetes-client/crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java
Lines 414 to 426 in 808084e
propertyOrAccessors.add(PropertyOrAccessor.fromProperty(property)); | |
Method method = potentialAccessors.get("is" + capitalized); | |
if (method != null) { | |
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name)); | |
} | |
method = potentialAccessors.get("get" + capitalized); | |
if (method != null) { | |
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name)); | |
} | |
method = potentialAccessors.get("set" + capitalized); | |
if (method != null) { | |
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name)); | |
} |
in some cases, the PropertyFacade
is extracting the field name from constructor/getter/setter
and this implementation will not respect the extracted name as opposed to the underlying field name.
We can either refactor the logic and use the PropertyFacade
name extraction logic everywhere or simply document this different behavior.
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 see, and the only way how that can happen is a @JsonProperty
on the field itself or getter (get* or is*) or setter.
I can only say that to me, as the user of the library, would never occur that fieldName
could reference the name after transformations.
Maybe it's because the word "field" is used, whose meaning in Java is well defined and is different from the term "property". (Field is a class variable, property is a getter/setter pair - which often is backed by a field). Also the annotation is @JsonProperty
, not "JsonField".
Or maybe it's because the reason I am dealing with @SchemaSwaps
in the first place is that I cannot place annotations (like @JsonProperty
) on the field directly.
So with your permission, I will add Javadocs on @SchemaSwap
, describing the currently implemented behaviour.
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.
To me, this "limitation" is totally fine, please just make it explicit in this doc:
https://github.com/fabric8io/kubernetes-client/blob/master/doc/CRD-generator.md#iofabric8crdgeneratorannotationschemaswap
mentioning something along the lines:
the name of the field is restricted to the original
fieldName
and should be backed by a matching concrete field of the matching class. Getters, setters, and constructors are not taken into consideration.
cc. @metacosm to check if he is on board with this decision.
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 that should be fine but this should be documented as a breaking change, though…
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 agree that it "might" be breaking, but, at the same time, this PR is solving some fundamental issues with the implementation and I doubt anyone else is using it just yet.
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.
Maybe we can add a line to the Changelog for reference, wdyt?
crd-generator/api/src/main/java/io/fabric8/crd/generator/InternalSchemaSwaps.java
Outdated
Show resolved
Hide resolved
crd-generator/api/src/test/java/io/fabric8/crd/generator/v1/JsonSchemaTest.java
Show resolved
Hide resolved
crd-generator/api/src/test/java/io/fabric8/crd/generator/v1/JsonSchemaTest.java
Show resolved
Hide resolved
I have pushed one more test for deeply nested hierarchies, which also tests that the Swap is applied multiple times. |
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.
@xRodney can you please fix the issues with the CI? |
0d15b10
to
671e70d
Compare
Hey @andreaTP, thanks to you, too for your help. |
Hi @andreaTP, it appears that you need to run the CI for me, with me being a first-time contributor? If so, could you please do it, so that we know if we are in the green? |
7025175
to
5280859
Compare
@@ -239,11 +256,9 @@ private T internalFromImpl(TypeDef definition, Set<String> visited, List<Interna | |||
continue; | |||
} | |||
|
|||
final PropertyFacade facade = new PropertyFacade(property, accessors, currentSchemaSwaps); | |||
ClassRef potentialSchemaSwap = schemaSwaps.lookupAndMark(definition.toReference(), name).orElse(null); |
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 that should be fine but this should be documented as a breaking change, though…
5280859
to
8595d41
Compare
I have updated the changelog and added the breaking change as requested. In the latest iteration I have also added a few more tests about the case when targetType is void. And also rebased. |
@xRodney we merged another PR slightly conflicting with the changes in this PR. Ping me and I will run the CI to validate and eventually move this forward 👍 thanks a lot for your effort! |
8595d41
to
c7bcba4
Compare
Sure, it's done |
Unfortunately, the CI failure seems to be legit, @xRodney can you have a look? |
@SchemaSwap is now repeatable, so multiple modifications can be applied from the same resource root. Fixed a bug when the swap was "used up" after just one usage, meaning that if the referenced type was used multiple times in object hierarchy, only the first one was swapped. Fixes fabric8io#4350
c7bcba4
to
74d533b
Compare
Oops, missing import. Sorry about that, it's fixed now. |
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.
LGTM, thanks!
Kudos, SonarCloud Quality Gate passed! |
thanks @xRodney ! |
Description
@SchemaSwap is now repeatable, so multiple modifications can be applied from the same resource root.
Fixed a bug when the swap was "used up" after just one usage, meaning that if the referenced type was used multiple times in object hierarchy, only the first one was swapped.
Fixes #4350
Type of change
test, version modification, documentation, etc.)
Checklist