Support external config file for eclipselink #88

aihuaxu · 2024-08-05T00:45:27Z

Description

Due to the limitation of eclipselink which requires the config file to be a resource inside a jar so a config file like /tmp/persistence.xml can't be supported directly. Persistence.createEntityManagerFactory() implementation searches the configuration file, checks if it's an archive (zip or jar file) and fails if it's not.

To workaround the issue, the PR supports the external configuration file format /tmp/conf.jar!/persistence.xml, adds /tmp/conf.jar in the classpath and then load the configuration. Here are the steps:

create a jar file from persistence.xml through jar -cf /tmp/conf.jar ./persistence.xml
configure eclipselink in polaris-server.yml

       metaStoreManager:
            type: eclipse-link
             persistence-unit: polaris
             conf-file: /tmp/conf.jar!/persistence.xml

The extra step is to run is jar -cf /tmp/conf.jar ./persistence.xml after updating the file.

Also updated persistence_unit name that the one for test should be polaris-dev and the default one for production should be polaris in persistence.xml.

Fixes #45

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

By generating a conf.jar, run test PolarisEclipseLinkMetaStoreTest.java with confFile =/tmp/ conf.jar!/persistence.xml
Add test coverage in PolarisEclipseLinkMetaStoreTest.java

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

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.

eric-maynard · 2024-08-05T17:04:58Z

If we can't support persistence.xml files outside of a JAR on the classpath, I'm skeptical as to whether we should do this. It seems like this is asking users to have a pretty strange configuration

sfc-gh-aixu · 2024-08-05T17:44:31Z

If we can't support persistence.xml files outside of a JAR on the classpath, I'm skeptical as to whether we should do this. It seems like this is asking users to have a pretty strange configuration

Yeah. It's awkward to do such configuration and it can only be used for workaround. One more improvement we can do is to dynamically add the jar to classpath so the user doesn't need to do that.

sfc-gh-mcollado · 2024-08-05T18:47:27Z

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

+      InputStream input = null;
+      int splitPosition = confFile.indexOf("!/");
+      if (splitPosition == -1) {
+        input = this.getClass().getClassLoader().getResourceAsStream(confFile);


Typically, I use Thread.currentThread().contextClassLoader() so that if the plugin jar happens to be loaded by a child classloader, rather than this one, we can still find it.

Yes. I made the change to create a new ClassLoader with the conf jar and I'm using Thread.currentThread().contextClassLoader() now.

sfc-gh-mcollado · 2024-08-05T18:54:27Z

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

+        String jarPrefixPath = confFile.substring(0, splitPosition);
+        String descPath = confFile.substring(splitPosition + 2);
+        URL prefixUrl = this.getClass().getClassLoader().getResource(jarPrefixPath);
+        URLClassLoader child =
+            new URLClassLoader(new URL[] {prefixUrl}, this.getClass().getClassLoader());
+        input = child.getResourceAsStream(descPath);


I guess I don't understand what's going on. This InputStream is not being used to setup eclipselink? Cause this InputStream could be coming from anywhere. Why is there a restriction that the resource is on the classpath?

This is used to parse it, but then EclispeLink again tries to load the resource directly when we call Persistence.createEntityManagerFactory

Since each realm has its only database, I have the following configuration in persistence.xml. I read this configuration and replace the realm then pass it to Persistence.createEntityManagerFactory() which will again read persistence.xml for the rest of the configurations. Persistence.createEntityManagerFactory() assumes the persistence.xml is a resource file inside a jar.

<property name="jakarta.persistence.jdbc.url" value="jdbc:h2:file:/tmp/eclipseLink/{realm}"/>

Due to the limitation of eclipselink, the config file has to be a resource inside a jar so a config file like /tmp/persistence.xml can't be supported directly. To workaround the issue, 1. create a jar file from persistence.xml through 'jar /tmp/cf conf.jar ./persistence.xml' 3. configure eclipselink in polaris-server.yml metaStoreManager: type: eclipse-link persistence-unit: polaris conf-file: /tmp/conf.jar!/persistence.xml

guitcastro · 2024-08-06T17:28:02Z

I was able to fix this in #95 .

collado-mike

Hmm, hacky but... it works I guess

collado-mike · 2024-08-06T23:55:58Z

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

+        }
+        URLClassLoader currentClassLoader =
+            new URLClassLoader(new URL[] {prefixUrl}, this.getClass().getClassLoader());
+        Thread.currentThread().setContextClassLoader(currentClassLoader);


Let's add a logging statement here stating that we're changing the classloader temporarily

collado-mike · 2024-08-06T23:56:35Z

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

+        String jarPrefixPath = confFile.substring(0, splitPosition);
+        confFile = confFile.substring(splitPosition + 2);
+        URL prefixUrl = this.getClass().getClassLoader().getResource(jarPrefixPath);
+        if (prefixUrl == null) {
+          prefixUrl = new File(jarPrefixPath).toURI().toURL();
+        }


Add logs here specifying the jar we found and how we're altering the classpath

Didn't see the comments before merging. Let me add that in a follow up.

collado-mike · 2024-08-06T23:56:55Z

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

+    } catch (Exception e) {
+      throw new RuntimeException(e);
+    } finally {


catch RuntimeException separately and just rethrow

collado-mike · 2024-08-06T23:57:40Z

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

+          String.format(
+              "Cannot find or parse the configuration file %s for persistence-unit %s",
+              confFile, persistenceUnitName);
+      LOG.error(str);


log the original exception so we can see the stacktrace

snazy · 2024-08-07T12:52:00Z

@aihuaxu @collado-mike I think, we need a follow-up of this one, because it adds binary content (a jar file).
We can generate a jar in a temp directory - but having binaries in Git isn't great.

sfc-gh-aixu · 2024-08-07T16:19:50Z

@aihuaxu @collado-mike I think, we need a follow-up of this one, because it adds binary content (a jar file). We can generate a jar in a temp directory - but having binaries in Git isn't great.

Ack. Let me think of a way to address that since that is only used for testing.

aihuaxu requested a review from a team as a code owner August 5, 2024 00:45

aihuaxu requested review from dennishuo, emkornfield and collado-mike August 5, 2024 00:52

sfc-gh-mcollado reviewed Aug 5, 2024

View reviewed changes

aihuaxu force-pushed the aixu-external-eclipselink-conf-file branch from e6cacd2 to 9e925cd Compare August 6, 2024 00:29

aihuaxu mentioned this pull request Aug 6, 2024

Allow eclipselink config outside classpath #95

Closed

13 tasks

aihuaxu force-pushed the aixu-external-eclipselink-conf-file branch from 9e925cd to 19667fe Compare August 6, 2024 04:02

aihuaxu mentioned this pull request Aug 6, 2024

[DOCUMENTATION]: missing documentation on how to load conf-file from other locations #45

Closed

MonkeyCanCode mentioned this pull request Aug 6, 2024

Implement missing doc "Configuring Polaris for Production" #43

Merged

2 tasks

aihuaxu force-pushed the aixu-external-eclipselink-conf-file branch from 19667fe to 023bb41 Compare August 6, 2024 15:38

aihuaxu requested review from eric-maynard and sfc-gh-mcollado August 6, 2024 16:34

eric-maynard approved these changes Aug 6, 2024

View reviewed changes

collado-mike approved these changes Aug 6, 2024

View reviewed changes

aihuaxu merged commit 4af03b6 into apache:main Aug 7, 2024
3 checks passed

sfc-gh-aixu mentioned this pull request Aug 7, 2024

Do not package test jar in the code base #108

Closed

MonkeyCanCode mentioned this pull request Aug 15, 2024

Add helm support #135

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support external config file for eclipselink #88

Support external config file for eclipselink #88

aihuaxu commented Aug 5, 2024 •

edited

Loading

eric-maynard commented Aug 5, 2024

sfc-gh-aixu commented Aug 5, 2024

sfc-gh-mcollado Aug 5, 2024

sfc-gh-aixu Aug 6, 2024

sfc-gh-mcollado Aug 5, 2024

eric-maynard Aug 5, 2024

sfc-gh-aixu Aug 6, 2024 •

edited

Loading

guitcastro commented Aug 6, 2024

collado-mike left a comment

collado-mike Aug 6, 2024

collado-mike Aug 6, 2024

aihuaxu Aug 7, 2024 •

edited

Loading

collado-mike Aug 6, 2024

collado-mike Aug 6, 2024

snazy commented Aug 7, 2024

sfc-gh-aixu commented Aug 7, 2024

Support external config file for eclipselink #88

Support external config file for eclipselink #88

Conversation

aihuaxu commented Aug 5, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

eric-maynard commented Aug 5, 2024

sfc-gh-aixu commented Aug 5, 2024

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

sfc-gh-aixu Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

guitcastro commented Aug 6, 2024

collado-mike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aihuaxu Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy commented Aug 7, 2024

sfc-gh-aixu commented Aug 7, 2024

aihuaxu commented Aug 5, 2024 •

edited

Loading

sfc-gh-aixu Aug 6, 2024 •

edited

Loading

aihuaxu Aug 7, 2024 •

edited

Loading