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.

Sign up for GitHub

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

Jump to bottom

Allow resource references in persistence xml datasource #24964

Merged
merged 8 commits into from
Jun 23, 2024
Merged

Allow resource references in persistence xml datasource #24964

merged 8 commits into from
Jun 23, 2024

Conversation

OndroMih
Copy link
Contributor

Allows to use a resource reference in persistence.xml, as in the provided test. This reference can be changed using resource-ref in glassfish-web.xml with alternative runtime descriptor or in a deployment plan during deployment.

dmatej
dmatej reviewed May 26, 2024
View reviewed changes
...src/test/java/org/glassfish/main/test/app/synchronize/SynchronizeStandaloneInstanceTest.java Outdated
new File("src/main/resources/org/glassfish/main/test/app/synchronize/sample.json"),
"resources/sample.json")
.addAsWebResource(new File("src/main/resources/org/glassfish/main/test/app/synchronize/index.html"));
.addAsWebInfResource(getTestResourceStatic("sample.json"), "resources/sample.json")
Copy link
Contributor

@dmatej dmatej May 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You don§t need to create a method for that, you can even shorten it to the resource reference. It is also better if you want to filter the file by maven.
It is always wrong to touch src/main/resources from the test in runtime (also Eclipse uses different basedir than Maven).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got you. Tests should get resources from the classpath instead of from the source directory. I updated the code.

dmatej
dmatej reviewed May 26, 2024
View reviewed changes
...ver/persistence/common/src/main/java/org/glassfish/persistence/common/PersistenceHelper.java
try {
return referenceContainer.getResourceReferenceByName(dataSourceName.toString()).getJndiName();
} catch (IllegalArgumentException e) {
return dataSourceName;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to log the exception as a warning at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here. This is a valid condition - if the datasource name is a proper JNDI and not a reference, just use it without any change. It would only pollute logs if it was a warning. I can add a debug logging.

The getResourceReferenceByName gives an exception if resource not found. Ideally, I would rather use findResourceReferenceByName, which returns a null, but that one is protected and only in WebBundleDescriptor, so I rather used a method on the interface ResourceReferenceContainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, debug/trace is good then. Sometimes I am hunting things around DOL and XML/annotations and any such log is useful.

@OndroMih OndroMih changed the title Ondro fix mapped ds persistence xml Allow resorce references in persistence xml datasource May 26, 2024
@dmatej
Copy link
Contributor

dmatej commented May 27, 2024

Seems something is still missing:

Expected: asadmin succeeded
     but: was <Command deploy failed.
remote failure: Error occurred during deployment: Exception while deploying the app [JtaDataSourceResourceRefTestWebApp] : Exception [EclipseLink-4002] (Eclipse Persistence Services - 4.0.2.v202306161219): org.eclipse.persistence.exceptions.DatabaseException
Internal Exception: java.sql.SQLException: Failed to obtain/create connection from connection pool [DerbyPool]. Reason: Connection creation failed for 1 times.
Error Code: 0. Please see server.log for more details.

>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.glassfish.main.test.app.persistence.resourceref.JtaDataSourceResourceRefTest.testDeploy(JtaDataSourceResourceRefTest.java:66)

@OndroMih
Copy link
Contributor Author

Hm, obviously, the Derby DB is not running, it's not started by the test suite. I'll try to set up the defalt datasource to point to an embedded database.

@arjantijms arjantijms changed the title Allow resorce references in persistence xml datasource Allow resource references in persistence xml datasource Jun 2, 2024
@OndroMih
Copy link
Contributor Author

Some EJB test failed in CI with no info about it. @dmatej , can you please have a look and see what's wrong with it?

@arjantijms
Copy link
Contributor

It does seem " ejb_group_1 / ejb_group_1.ejb-ejb30-hello-session3. ejb-ejb30-hello-session3ID" fails structurally. @OndroMih did you try to run it locally?

@OndroMih
Copy link
Contributor Author

@arjantijms , I ran locally after merged from master, and I get 10 failed tests when I run ./runtests.sh ejb_group_1

From the test_results_junit.xml file:

<testsuite tests="103" failures="10" errors="0" name="ejb_group_1">

I'll try to look what's happening.

It's weird that when I ran the test without merging master, all tests pass but there are only 110 tests.

The latest Jenkins build for this PR reports 111 tests:

<testsuite tests="111" failures="0" errors="1" name="ejb_group_1">

And the one that is failing is missing in the 110 tests when I run my branch.

So, my branch is based on master, which only has 110 tests. The latest Jenkins run for this PR has 111 tests. And now the master branch has 113 tests. Were any tests added to the ejb_group_1 test suite in the past few weeks? Or why do I get different number of tests?

@OndroMih OndroMih force-pushed the ondro-fix-mapped-DS-persistence-xml branch from 4fb25d5 to 592fa1f Compare June 20, 2024 23:59
@OndroMih
Copy link
Contributor Author

I got more familiar with how the tests are set up, and then I found the culprit. In the appclient context, there's a null value for context where I didn't expect null on server.

@OndroMih
Mapped JTA DS: Null pointer check when in appclient
b018e55
@OndroMih
Mapped JTA DS: Refactoring
2d9d392 
Cleanup, added javadoc, moved a helper class to the test tools module
@OndroMih
Copy link
Contributor Author

@dmatej , @arjantijms , please review.

@arjantijms
Copy link
Contributor

Looks good to me now!

@arjantijms arjantijms merged commit fba69b1 into eclipse-ee4j:master Jun 23, 2024
2 checks passed
@dmatej dmatej added this to the 7.0.16 milestone Jul 4, 2024
@dmatej dmatej added the enhancement New feature or request label Jul 4, 2024
@dmatej dmatej mentioned this pull request Jul 6, 2024
Merged
@OndroMih OndroMih deleted the ondro-fix-mapped-DS-persistence-xml branch August 12, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmatej dmatej dmatej left review comments

@arjantijms arjantijms arjantijms approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
7.0.16
Development

Successfully merging this pull request may close these issues.
3 participants
@OndroMih @dmatej @arjantijms