Skip to content
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

[JBPM-9558] KIE using XStream does not complain on unknown fields #2369

Merged
merged 1 commit into from Jan 20, 2021

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Jan 13, 2021

In order to "relax" deserialization requirements, user can specifiy as parameter of contentype, ignoreUnknown=true, which will ignore unknown fields durign xstream deserialization.

JIRA:

link

Depends on
kiegroup/kie-soup#173

@fjtirado fjtirado marked this pull request as draft January 13, 2021 12:10
@fjtirado fjtirado force-pushed the JBPM-9558 branch 5 times, most recently from e968f70 to a9c33a1 Compare January 13, 2021 14:59
@fjtirado fjtirado marked this pull request as ready for review January 13, 2021 17:35
@fjtirado fjtirado force-pushed the JBPM-9558 branch 2 times, most recently from c3761e5 to ddd70b3 Compare January 13, 2021 18:06
@elguardian
Copy link
Member

marshaller is created upon deployment and cached so there is not need for multithread. You can add user space objects during creation to the marshaller and ignore them as upon deployment you will have access to any java class in the kjar

Copy link
Member

@elguardian elguardian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs another approach

@fjtirado
Copy link
Contributor Author

needs another approach

Changed to use Custom mapper and thread local

@fjtirado fjtirado force-pushed the JBPM-9558 branch 3 times, most recently from d51c0c5 to 4171af3 Compare January 14, 2021 12:40
@fjtirado
Copy link
Contributor Author

@elguardian Ready to re-review

Copy link
Member

@elguardian elguardian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor clean up

Copy link
Contributor

@afalhambra afalhambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment, others are just cosmetic changes here.

@@ -35,7 +35,8 @@ public Marshaller build(Set<Class<?>> classes, MarshallingFormat format, ClassLo
return new XStreamMarshaller(classes, classLoader) {
@Override
protected void buildMarshaller(Set<Class<?>> classes, ClassLoader classLoader) {
xstream = XStreamUtils.createNonTrustingXStream(new PureJavaReflectionProvider(), new DomDriver("UTF-8", new XmlFriendlyNameCoder("_-", "_")));
xstream = XStreamUtils.createNonTrustingXStream(new PureJavaReflectionProvider(), new DomDriver("UTF-8", new XmlFriendlyNameCoder("_-", "_"))
, next -> new CustomElementIgnore(next));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only way to get a buildMarshaller with this ignoreUnknownFields feature is by using and passing this new CustomElementIgnore - so I'm just thinking whether it would be worth to make it public instead of protected - otherwise we are forcing customer to create a package with the same structure - or am I wrong? wdyt?

Copy link
Contributor Author

@fjtirado fjtirado Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is intended to be used only in inherited classes, so protected is right

Adding ElementIgnoringMapper custom implementation.
@sonarcloud
Copy link

sonarcloud bot commented Jan 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@afalhambra afalhambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @fjtirado!

@fjtirado
Copy link
Contributor Author

@elguardian I think we are ready to go

Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a comment in the test for adding false flag

@elguardian elguardian merged commit 4ae7b1d into kiegroup:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants