-
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
Removes use of JAXB API via jackson-dataformat-xml #29384
Conversation
a9c5fdd
to
b81d060
Compare
b81d060
to
5b63b51
Compare
5b63b51
to
97edd1c
Compare
@Getter | ||
public final class JDBCRepositorySQL { | ||
|
||
@XmlAttribute(required = true) | ||
@JacksonXmlProperty(isAttribute = true) |
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.
We lost required
here? Is there no such property with JacksonXmlProperty
?
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.
- There is no direct equivalent.
Unfortunately XML is special: due to structure being what it is, empty Element looks either as "Object with no properties" or "empty String".
- In Required attribute of
@JsonProperty
is ignored when deserializing from XML FasterXML/jackson-dataformat-xml#538, the Jackson maintainer has a different opinion onrequired
than the JAXB API. Should I use@JsonProperty
to markrequired
and then set it as follows?
final XmlMapper xmlMapper = new XmlMapper();
xmlMapper.configure(DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES, true);
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'm not sure how the JAXB API handles the
required
attribute. -
In my local unit test,
jaxb-runtime
does not throw an exception for empty elements, but gets null values normally likejackson-dataformat-xml
.required
seems to be just for IDE censorship.
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.
OK, I see. It could be better to open an issue since we need more investigation.
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.
OK, I see. It could be better to open an issue since we need more investigation.
For #26041.
Changes proposed in this pull request:
shardingsphere-transaction-xa-narayana
does not require JAXB at all.org.glassfish.jaxb:jaxb-runtime:2.3.1
which is no longer needed. Passed the test of./mvnw -PnativeTestInShardingSphere -T1C -e clean test
. For Improve GraalVM Reachability Metadata and corresponding nativeTest related unit tests #29052 .com.fasterxml.jackson.datatype:jackson-datatype-jdk8
to adapt to Jackson 2.16.0. Otherwise an error is thrown.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
.