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

Adding support for private fields in @BeanParam #5525

Merged
merged 3 commits into from Mar 7, 2024

Conversation

divyanshshekhar
Copy link

@divyanshshekhar divyanshshekhar commented Feb 4, 2024

Purpose

Added support for private fields in POJOs used as BeanParam

Issue / Need

The jersey server already supports classes with private fields annotated QueryParam, HeaderParam etc to be used as BeanParam in the JAX-RS API interface. e.g.
Given a POJO representing an employee:

public class Employee {
    private String id;
    private String firstName;

    public Employee() {
    }

    public String getId() {
        return id;
    }

    public void setId(String id) {
        this.id = id;
    }

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }
}

We can use the above class as @BeanParam in our service interface as below

    @GET
    @Path("employees")
    List<Employee> findEmployee(@BeanParam FindEmployeeRequest findEmployeeRequest);

The above works with Jersey server.
However, the same class and interface cannot be used with jersey-proxy-client as of now because the jersey-proxy-client, upon encountering fields with private fields having getters failes with following error:

java.lang.IllegalAccessException: class org.glassfish.jersey.client.proxy.RequestParameters cannot access a member of class com.dsgaur.example.jersey.api.FindEmployeeRequest with modifiers "private"
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:394)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:714)
	at java.base/java.lang.reflect.Field.checkAccess(Field.java:1156)
	at java.base/java.lang.reflect.Field.get(Field.java:441)
	at org.glassfish.jersey.client.proxy.RequestParameters.addBeanParameter(RequestParameters.java:160)
	at org.glassfish.jersey.client.proxy.RequestParameters.addParameter(RequestParameters.java:139)
	at org.glassfish.jersey.client.proxy.WebResourceFactory.invoke(WebResourceFactory.java:203)

This breaks the compatibility of having a common API module with JAX-RS annotated service interfaces and POJOs shared by the server application and the various client applications which might be using the same service. This is a typical usecase.

Why it fails

It is not checking if the field is accessible before calling Field.get(beanParam), which causes the above error. This statement is reached when a class being used as @BeanParam has annotations on the field declaration but the field is private.

Fix

Check if the field is accessible in the current beanParam object before calling get(). If the field is accessible, get the value, otherwise use the getter for that field if present.

Affected Versions:

3.x
4.x

Fixed #5526

Signed-off-by: Divyansh Shekhar Gaur divyanshshekhar@users.noreply.github.com

Signed-off-by: Divyansh Shekhar Gaur <divyanshshekhar@users.noreply.github.com>
Signed-off-by: Divyansh Shekhar Gaur <divyanshshekhar@users.noreply.github.com>
Signed-off-by: Divyansh Shekhar Gaur <divyanshshekhar@users.noreply.github.com>
@jansupol
Copy link
Contributor

A universal solution would likely be to set the field.setAccesible(true) so that the getter method is not required for the functionality. That's what all the reflection frameworks do.

@divyanshshekhar
Copy link
Author

@jansupol I thought of that as well. However, if we do that, then if someone has defined the annotations on both the field and the getter, then the annotations on the getter would not be detected as it wouldn't go into else block.

Let me know your thoughts on this.

@jansupol
Copy link
Contributor

You want to override the field value by the getter. But it is inconsistent, the getter overrides the value only for the private fields, and not for the public ones.

@divyanshshekhar
Copy link
Author

I think if the POJO classes are written as per convention, there should either be public fields or getters but not both. In that case this would work as expected.

@jansupol jansupol merged commit c12c0dd into eclipse-ee4j:3.1 Mar 7, 2024
3 checks passed
@ivan-slavchev
Copy link

Hi @divyanshshekhar, just FYI, AccessibleObject.canAccess throws exception when there are static fields inside the POJO with the current implementation. I don't know if this is a valid case, but maybe it is worth considering. The reason for the exception is that the obj must be null in case of static fields. From the javadoc of the method:

     * Test if the caller can access this reflected object. If this reflected
     * object corresponds to an instance method or field then this method tests
     * if the caller can access the given {@code obj} with the reflected object.
     * For instance methods or fields then the {@code obj} argument must be an
     * instance of the {@link Member#getDeclaringClass() declaring class}. For
     * static members and constructors then {@code obj} must be {@code null}.

Here is a part of the stack trace for reference:

java.lang.IllegalArgumentException: non-null object for public static final int **************
	at java.base/java.lang.reflect.AccessibleObject.canAccess(AccessibleObject.java:494) ~[?:?]
	at de.bonprix.jersey.lib.RequestParameters.addBeanParameter(RequestParameters.java:159)

@senivam senivam added this to the 3.1.6 milestone Mar 11, 2024
@jansupol
Copy link
Contributor

@ivan-slavchev Would you care to create a patch?

@ivan-slavchev
Copy link

@jansupol here is a patch, I hope it helps:
use_only_non-static_fields_in_bean_param.patch

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

Successfully merging this pull request may close these issues.

None yet

4 participants