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

PAYARA-2267 JPA Conversion now unwraps connection properly #2279

Merged
merged 3 commits into from Mar 5, 2018

Conversation

Cousjava
Copy link
Contributor

No description provided.

@Cousjava Cousjava added this to the Payara 4.181 milestone Jan 16, 2018
@Cousjava
Copy link
Contributor Author

jenkins test please

return con;
public Connection getConnection() throws SQLException {
//To ensure that the actual connection is always returned and not a proxy
Connection result = con.unwrap(java.sql.Connection.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe blindly unwrapping on getConnection is the correct fix. I think the problem is why unwrap is failing. However I could be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is also missing //Portions Copyright comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unwrap was never called, and based on the comment above the method then the actual connection should be returned and not a wrapped version of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unwrap connection is not not used anywhere else in Payara or Eclipselink, unless its done via reflection.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also do not believe this is the correct fix.
unwrap() is called by the user to get access to underlying proprietary classes.
Proxy should be returned, not the raw connection here

@Cousjava
Copy link
Contributor Author

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@arjantijms
Copy link
Contributor

@smillidge Is this now okay to be merged?

@arjantijms arjantijms added the PR: DO NOT MERGE Don't merge PR until further notice label Feb 3, 2018
@arjantijms arjantijms removed this from the Payara 4.181 milestone Feb 13, 2018
Copy link
Contributor

@lprimak lprimak left a comment

Choose a reason for hiding this comment

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

This needs further review, as unwrap() isn't the correct solution in my opinion

@smillidge smillidge added this to the Payara 4.x milestone Feb 24, 2018
@Pandrex247 Pandrex247 changed the base branch from master to Payara4 March 5, 2018 15:49
@Pandrex247 Pandrex247 changed the base branch from Payara4 to master March 5, 2018 15:52
@Pandrex247 Pandrex247 merged commit 5f04340 into payara:master Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: DO NOT MERGE Don't merge PR until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants