Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates several Nexo model classes, including AbortRequest, DisplayRequest, LoginRequest, and MessageHeader, from JAXB XML annotations to GSON @SerializedName annotations. The changes also involve removing JAXB-specific metadata and adding unit tests to verify that serialization and deserialization remain functional even when fields previously marked as required are missing from the JSON payload. I have no feedback to provide.
|
@jeandersonbc is my understanding correct that we were using the jaxb annotation with GSON as the serializer / deserializer, so there is no actual change in behavior other than using GSON annotations? |
Correct @thomas-cools (cc @gcatanese)! I also pushed this test that verifies how GSON handles the JAXB annotations. The target class ( I'm going to proceed to JAXB -> Gson annontations as part of the original task. |
jeandersonbc
force-pushed
the
initial-migration-jaxb
branch
from
34c0d98 to
4ade44d
Compare
jeandersonbc
changed the title
jeandersonbc
force-pushed
the
initial-migration-jaxb
branch
from
4ade44d to
f0f5cce
Compare
gcatanese
left a comment
gcatanese
left a comment
There was a problem hiding this comment.
Great work Jean, this is a challenging issue.
I left few comments in the code and there is here one about test coverage, they might be edge cases so please assess whether they are applicable or not
Test Coverage Gaps
• No direct unit tests for
XMLEnumTypeAdapterwith @SerializedName, especially the edge case where the annotation is absent on an enum constant.
• No round-trip tests forSaleToPOIRequest— only SaleToPOIResponse is covered despite the same migration pattern.
•RepeatedMessageResponseBody/RepeatedResponseMessageBody— migrated but zero test coverage.
•TerminalAPIGsonBuilderfallback behavior — no test verifying the behavior when @SerializedName is missing.
| } | ||
| return field.getName(); | ||
| }; | ||
| FieldNamingStrategy fieldNamingStrategy = Field::getName; |
There was a problem hiding this comment.
This can be removed
FieldNamingStrategyis now a no-op (TerminalAPIGsonBuilder.java)
The strategy is set to Field::getName, which is Gson's default. Since all fields now use @SerializedName (which takes precedence), the strategy is dead code. The risk is that any future field added
without @SerializedName will silently fall back to the Java field name instead of the expected PascalCase JSON key. Recommend removing the setFieldNamingStrategy call entirely to make the contract explicit.
| @XmlElements({ | ||
| @XmlElement(name = "KeyTransport", type = KeyTransport.class), | ||
| @XmlElement(name = "KEK", type = KEK.class) | ||
| }) |
There was a problem hiding this comment.
Just a remark: keyTransportOrKEK starts with a lower case, unlike other attributes.
It is worth checking that we don't move from a case-insensitive approach (with XmlType) to case-sensitive (with SerializedName)
There was a problem hiding this comment.
Good catch, we can try this out with this payload and some test using it. The expectation is that everything different from keyTransportOrKEK should be ignored as an unknown field.
| @XmlElement(name = "KEK", type = KEK.class) | ||
| }) | ||
| @SerializedName("keyTransportOrKEK") | ||
| protected List<Object> keyTransportOrKEK; |
There was a problem hiding this comment.
It looks like here we can improve the deserialization, even if we deal with a polymorphic list
AuthenticatedData.keyTransportOrKEKloses type discrimination (AuthenticatedData.java)
The old @XmlElements annotation handled KeyTransport.class vs KEK.class polymorphic deserialization. With Gson, items in this list are deserialized as LinkedTreeMap (generic objects), not concrete types.
The test explicitly acknowledges this regression with assertInstanceOf(Object.class, ...). Any caller doing (KeyTransport) keyTransportOrKEK.get(0) will get a ClassCastException at runtime. Fix: register
a custom TypeAdapter or JsonDeserializer that inspects JSON keys (e.g., presence of KEKIdentifier vs RecipientIdentifier) to determine the concrete type.
It look like before this used to work
AuthenticatedData data = ...; // from JAXB unmarshalling
KeyTransport kt = (KeyTransport) data.getKeyTransportOrKEK().get(0); // safe cast
We can maybe create a test to confirm it.
The fix is a custom JsonDeserializer for AuthenticatedData (i.e. creating AuthenticatedDataDeserializer, registered in TerminalAPIGsonBuilder.
This PR removes the JAXB-bind related annotations. This change is needed due to Jakarta EE +9 compatibility. The existing code in `com.adyen.terminal.serialization` relies on GSON; thus, the JAXB annotations were replaced by the GSON's @SerializedName. The JAXB validations (e.g., type checks and required fields) were not processed by GSON and have no equivalent (see `AbortRequestTest` for instance). The migration was guided by test coverage and focused on serialization and deserialization of JSON payloads. POM XML changes: - removal of jaxb dependency - addition of plugin enforcement to block unwanted dependencies
jeandersonbc
force-pushed
the
initial-migration-jaxb
branch
from
f0f5cce to
e0d6100
Compare
4 participants
Description
This PR removes the JAXB-bind related annotations. This change is needed due to Jakarta EE +9 compatibility.
Since the underlying serializer relies on Gson lib, we are unable to use directly Jackson annotations since they don't work as expected. This was verified via test coverage. Furthermore, the
required=truevalidation was never enforced (see AbortRequestTest tests).The existing code in
com.adyen.terminal.serializationrelies on GSON; thus,the JAXB annotations were replaced by the GSON's @SerializedName.The migration was guided by test coverage and focused on serialization and deserialization of JSON payloads.
POM XML changes:
Tested scenarios