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

RANGER-4225: Possible Jackson serialization issue due to not comply with Java bean standards #252

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sercanCyberVision
Copy link

@sercanCyberVision sercanCyberVision commented May 5, 2023

What changes were proposed in this pull request?

@JsonProperty annotation has been added to model classes for mapping the properties with their corresponding getter/setter methods. This will not effect Ranger's functionality directly, but it will provide consistency in case there is Jackson jar conflict (or when/if Jackson is upgraded to version-2).
Please see the Jira for more detailed analysis of the issue https://issues.apache.org/jira/browse/RANGER-4225

Basically, some of the model classes do not comply with JavaBean naming conventions, therefore it is possible that Ranger may hit this issue https://stackoverflow.com/questions/30205006/why-does-jackson-2-not-recognize-the-first-capital-letter-if-the-leading-camel-c

As a reference of the issue, please see http://futuretask.blogspot.com/2005/01/java-tip-6-dont-capitalize-first-two.html

How was this patch tested?

Built the project and made sure that deserialized responses on UI side have correct property names even though Jackson-2 jars are in the classpath, hence the corresponding UI components work as expected.

@sercanCyberVision sercanCyberVision changed the title RANGER-4225: Possible Jackson serialization due to not complain to Java bean standards RANGER-4225: Possible Jackson serialization issue due to not comply to Java bean standards May 5, 2023
@sercanCyberVision sercanCyberVision changed the title RANGER-4225: Possible Jackson serialization issue due to not comply to Java bean standards RANGER-4225: Possible Jackson serialization issue due to not comply with Java bean standards May 5, 2023
@sercanCyberVision sercanCyberVision force-pushed the RANGER-4225 branch 2 times, most recently from 602a07d to 77fac4d Compare May 5, 2023 12:08
@sercanCyberVision
Copy link
Author

@kumaab could you please take a look at this?

@sercanCyberVision
Copy link
Author

@mneethiraj could you please take a look at this? I hit this issue earlier, and the solution here can save some grief for the end-users for the future.

@bhavikpatel9977
Copy link
Contributor

LGTM.

Kindly resolve the conflict.

@sercanCyberVision
Copy link
Author

@bhavikpatel9977, thank you for reviewing the PR. I have resolved the conflict and quickly tested it again.
So, two actions have been taken here:

  1. org.codehaus.jackson has been replaced with com.fasterxml.jackson.
  2. @JsonProperty has been added to view classes to help jackson recognize java bean properties.

@bhavikpatel9977
Copy link
Contributor

@mneethiraj Can you kindly review once

Copy link
Contributor

@mneethiraj mneethiraj left a comment

Choose a reason for hiding this comment

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

@sercanCyberVision - thank you for taking up this critical update to switch to fasterxml.jackson. In addition to the couple of review comments, can you please verify that the final packaging does not include codehaus jackson library? It seems Ranger admin continues to use codehaus jackson library to serialize REST API responses. For example, the response includes fields having null and empty value.

@@ -91,7 +91,7 @@
<cglib.version>2.2.0-b23</cglib.version>
<checkstyle.plugin.version>3.1.0</checkstyle.plugin.version>
<checkstyle.version>8.29</checkstyle.version>
<codehaus.jackson.version>1.9.13</codehaus.jackson.version>
<com.fasterxml.jackson.version>2.17.0</com.fasterxml.jackson.version> <!--here-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using existing property fasterxml.jackson.version (line # 215) instead of introducing new one com.fasterxml.jackson.version. Also, update the version in #215 and #216 from 2.14.0 to 2.17.0.

import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;


@JsonAutoDetect(getterVisibility = Visibility.NONE, setterVisibility = Visibility.NONE, fieldVisibility = Visibility.ANY)
Copy link
Contributor

Choose a reason for hiding this comment

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

With the move to fasterxml.jackson 2.x, consider replacing deprecated include = JsonSerialize.Inclusion.NON_EMPTY, with @JsonInclude(JsonInclude.Include.NON_EMPTY)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants