Skip to content

Conversation

@adutra
Copy link
Contributor

@adutra adutra commented Jan 6, 2025

I turns out it is actually possible to read a persistence.xml file directly from the filesystem.

The trick is that it must be in a directory that will be considered as the "archive directory"; the eclipselink.persistencexml property must point to the path of the file relative to the archive directory's parent. E.g. if the persistence file is in /config/persistence.xml, the archive directory is /config and the eclipselink.persistencexml property must be config/persistence.xml.

This PR builds on that observation and expands the allowed locations for a persistence.xml file to the following ones:

  1. Classpath resource
  2. Classpath resource, embedded in a jar
  3. Filesystem path
  4. Filesystem path, embedded in a jar

Note: this PR does not modify the Helm chart, but we could look into simplifying it to use plain files instead of jars.

Comment on lines +241 to +242
Arguments.of(confJar + "!/persistence.xml", true),
Arguments.of(confJar + "!/dummy.xml", false),
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I'm reading these correctly, this means the new code should work with existing config + jar setups?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's my understanding reading the code: it's "backward" compatible, fallbacking to the "inner" persistence xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's backwards-compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Great find

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be made at build time?.. or perhaps by test code itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly... it's just 1.5Kb though, is that worth the hassle?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that if EclipseLink config evolves, the jar would go out of sync with the production code... But this is not likely, I guess... I think it's ok to merge "as is",

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think in the future we should be making more stuff at run/test time (e.g. the python client code can be generated from the spec). But I agree that doesn't need to block this fix

@eric-maynard eric-maynard merged commit 6bda078 into apache:main Jan 6, 2025
5 checks passed
@adutra
Copy link
Contributor Author

adutra commented Jan 14, 2025

Related: #95.

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.

4 participants