Allow eclipselink config outside classpath #95

guitcastro · 2024-08-06T03:09:31Z

Description

As today, It's impossible to load configurations outside the classpath using eclipse-link. This PR allow do load config files from any path.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

I've update the PolarisEclipseLinkMetaStoreTest to ensure that the file is being loaded from outside the classpath.

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
I have signed and submitted the ICLA and if needed, the CCLA. See Contributing for details.

…fig_outside_classpath

aihuaxu · 2024-08-06T03:58:40Z

...o/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

@@ -124,7 +124,7 @@ private Map<String, String> loadProperties(String confFile, String persistenceUn
    }

    try {
-      InputStream input = this.getClass().getClassLoader().getResourceAsStream(confFile);
+      FileInputStream input = new FileInputStream(confFile);


This is fine but Persistence.createEntityManagerFactory(persistenceUnitName, properties) puts the limitation which can only loads the configuration from a jar. I'm making a change to workaround that. #88

I was able to fix this in the last commit. Please see the this comment in #79

I replied in another thread.

@guitcastro Actually it's not because of the properties. Of course we need the properties because we want to replace realm in the database name - each realm will have its own database.

Persistence.createEntityManagerFactory() will still load configuration file from resource even without properties.

@aihuaxu only if you specify the ECLIPSELINK_PERSISTENCE_XML property. I've tested and it worked.

From their docs:

This property is only used by EclipseLink when it is locating the configuration file.
When used within an EJB/Spring container in container managed mode the locating and reading of this file is done by the container and will not use this configuration.

I've added unit test to ensure config loads from outside the classpath is working.

@guitcastro Can you list the steps how you get it to work? It's possible the unit test found the persistence.xml from the embedded jar. That's why it doesn't fail for you.

I don't see the changes or unit tests in the PR. Did you forget to include that?

I only see
properties.put(ECLIPSELINK_PERSISTENCE_XML, confFile); -- removed

FileInputStream input = new FileInputStream(confFile);

@aihuaxu Sorry, I have forgot to push it. Can you please take a look now? I also added a test to ensure that the config is not being loaded from the classpath.

eric-maynard · 2024-08-06T04:57:58Z

This looks similar to #79, which did not work when tested.

guitcastro · 2024-08-06T17:27:38Z

This looks similar to #79, which did not work when tested.

I fix this in the last commit. Please see the this comment in #79

eric-maynard · 2024-08-06T19:01:35Z

Hi @guitcastro, I tested this change and found that it did not work. See my notes below:

I copied the default persistence.xml to my desktop and updated the polaris-server.yml to point to it:

. . .
metaStoreManager:
  type: eclipse-link
  persistence-unit: polaris-dev
  conf-file: /Users/emaynard/Desktop/persistence.xml
. . .

I then modified the default persistence.xml at .../resources/META-INF/persistence.xml to use a different persistence-unit -- polaris-fake instead of polaris-dev. When I attempt to bootstrap, I see this:

jakarta.persistence.PersistenceException: No Persistence provider for EntityManager named polaris-dev
	at jakarta.persistence.Persistence.createEntityManagerFactory(Persistence.java:86)
	at io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkMetaStoreSessionImpl.<init>(PolarisEclipseLinkMetaStoreSessionImpl.java:109)
. . .

If I revert the default persistence.xml back to using polaris-dev, it bootstraps normally.

Based on this, I suspect that your change is in fact just loading the default persistence.xml which is on the classpath, and that it does not actually allow us to load a persistence.xml outside of the classpath. This makes sense when you consider your change is deleting the config that is supposed to point to the location of the persistence.xml. Is it possible you were observing the same in your testing?

guitcastro · 2024-08-06T19:18:31Z

Hi @guitcastro, I tested this change and found that it did not work. See my notes below:

I copied the default persistence.xml to my desktop and updated the polaris-server.yml to point to it:
. . .
metaStoreManager:
  type: eclipse-link
  persistence-unit: polaris-dev
  conf-file: /Users/emaynard/Desktop/persistence.xml
. . .
I then modified the default persistence.xml at .../resources/META-INF/persistence.xml to use a different persistence-unit -- polaris-fake instead of polaris-dev. When I attempt to bootstrap, I see this:
jakarta.persistence.PersistenceException: No Persistence provider for EntityManager named polaris-dev
	at jakarta.persistence.Persistence.createEntityManagerFactory(Persistence.java:86)
	at io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkMetaStoreSessionImpl.<init>(PolarisEclipseLinkMetaStoreSessionImpl.java:109)
. . .
If I revert the default persistence.xml back to using polaris-dev, it bootstraps normally.

Based on this, I suspect that your change is in fact just loading the default persistence.xml which is on the classpath, and that it does not actually allow us to load a persistence.xml outside of the classpath. This makes sense when you consider your change is deleting the config that is supposed to point to the location of the persistence.xml. Is it possible you were observing the same in your testing?

Maybe the /Users/emaynard/Desktop/persistence.xml is not defining a polaris-dev. I just commit two unit test that ensure that the default config is not being loaded. If you test the lasted version it should at least throw an exception instead of loading the default one.

eric-maynard · 2024-08-06T19:41:48Z

...java/com/snowflake/polaris/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreTest.java

+  }
+
+  @Test
+  void ensureNotLoadingDefaultClassPersistence() {


How does this test that you're not still loading the file from /META-INF/persistence.xml?

eric-maynard · 2024-08-06T19:42:23Z

No @guitcastro it was definitely there -- like I mentioned, I copied it directly from the existing default file. Have you done an end-to-end test using this change including bootstrapping Polaris? If so can you share the steps?

aihuaxu · 2024-08-06T19:42:58Z

...java/com/snowflake/polaris/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreTest.java

    PolarisDiagnostics diagServices = new PolarisDefaultDiagServiceImpl();
    PolarisEclipseLinkStore store = new PolarisEclipseLinkStore(diagServices);
    PolarisEclipseLinkMetaStoreSessionImpl session =
        new PolarisEclipseLinkMetaStoreSessionImpl(
-            store, Mockito.mock(), () -> "realm", null, "polaris-dev");
+            store, Mockito.mock(), () -> "realm", tmpFile.toString(), "polaris-dev");


For this, Persistence.createEntityManagerFactory() will use the one in the jar, not the /tmp/%s-persistence.xml.

You can try to remove persistence.xml from the project and see the issue.

guitcastro · 2024-08-06T21:26:54Z

@aihuaxu @eric-maynard You are right, what happened is that my external configuration had a persistence unit also named polaris-dev and got merged with the one in the classpath. Thus my jakarta.persistence.jdbc.url took precedence from the class path and I was able to connect with my database (postgres).

sfc-gh-aixu · 2024-08-06T22:36:56Z

@aihuaxu @eric-maynard You are right, what happened is that my external configuration had a persistence unit also named polaris-dev and got merged with the one in the classpath. Thus my jakarta.persistence.jdbc.url took precedence from the class path and I was able to connect with my database (postgres).

Yeah. So let's move forward with my workaround solution for now until we have a better one?

guilhermecastro added 2 commits August 6, 2024 00:01

allow load eclipse-link configs from outside classpath

27bc0e1

Merge remote-tracking branch 'origin/main' into allow_eclipselink_con…

9a05ce4

…fig_outside_classpath

guitcastro requested a review from a team as a code owner August 6, 2024 03:09

aihuaxu reviewed Aug 6, 2024

View reviewed changes

removed ECLIPSELINK_PERSISTENCE_XML property

cfc596c

guitcastro mentioned this pull request Aug 6, 2024

Support external config file for eclipselink #88

Merged

15 tasks

aihuaxu mentioned this pull request Aug 6, 2024

Support user-specified paths for conf-file in the eclipse-link metastore manager #79

Closed

9 tasks

guilhermecastro added 2 commits August 6, 2024 16:12

unit test

bfe2286

add test to ensure that is not loading the default class path

13831f7

eric-maynard reviewed Aug 6, 2024

View reviewed changes

aihuaxu reviewed Aug 6, 2024

View reviewed changes

guitcastro closed this Aug 7, 2024

adutra mentioned this pull request Jan 14, 2025

EclipseLink: allow reading persistence.xml from the filesystem #613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow eclipselink config outside classpath #95

Allow eclipselink config outside classpath #95

guitcastro commented Aug 6, 2024 •

edited

Loading

aihuaxu Aug 6, 2024

guitcastro Aug 6, 2024 •

edited

Loading

aihuaxu Aug 6, 2024

guitcastro Aug 6, 2024

guitcastro Aug 6, 2024

aihuaxu Aug 6, 2024 •

edited

Loading

guitcastro Aug 6, 2024

eric-maynard commented Aug 6, 2024 •

edited

Loading

guitcastro commented Aug 6, 2024 •

edited

Loading

eric-maynard commented Aug 6, 2024

guitcastro commented Aug 6, 2024

eric-maynard Aug 6, 2024

eric-maynard commented Aug 6, 2024

aihuaxu Aug 6, 2024

aihuaxu Aug 6, 2024

guitcastro commented Aug 6, 2024

sfc-gh-aixu commented Aug 6, 2024

Allow eclipselink config outside classpath #95

Allow eclipselink config outside classpath #95

Conversation

guitcastro commented Aug 6, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

guitcastro Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aihuaxu Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard commented Aug 6, 2024 • edited Loading

guitcastro commented Aug 6, 2024 • edited Loading

eric-maynard commented Aug 6, 2024

guitcastro commented Aug 6, 2024

Choose a reason for hiding this comment

eric-maynard commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guitcastro commented Aug 6, 2024

sfc-gh-aixu commented Aug 6, 2024

guitcastro commented Aug 6, 2024 •

edited

Loading

guitcastro Aug 6, 2024 •

edited

Loading

aihuaxu Aug 6, 2024 •

edited

Loading

eric-maynard commented Aug 6, 2024 •

edited

Loading

guitcastro commented Aug 6, 2024 •

edited

Loading