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

Allow eclipselink config outside classpath #95

Closed
wants to merge 5 commits into from
Closed

Allow eclipselink config outside classpath #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
27bc0e1
allow load eclipse-link configs from outside classpath
Aug 6, 2024
9a05ce4
Merge remote-tracking branch 'origin/main' into allow_eclipselink_con…
Aug 6, 2024
cfc596c
removed ECLIPSELINK_PERSISTENCE_XML property
Aug 6, 2024
bfe2286
unit test
Aug 6, 2024
13831f7
add test to ensure that is not loading the default class path
Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
*/
package io.polaris.extension.persistence.impl.eclipselink;

import static org.eclipse.persistence.config.PersistenceUnitProperties.ECLIPSELINK_PERSISTENCE_XML;
import static org.eclipse.persistence.config.PersistenceUnitProperties.JDBC_URL;

import com.google.common.base.Predicates;
import com.google.common.collect.Maps;
import io.polaris.core.PolarisCallContext;
import io.polaris.core.context.RealmContext;
import io.polaris.core.entity.PolarisBaseEntity;
Expand Down Expand Up @@ -47,7 +45,7 @@
import jakarta.persistence.EntityTransaction;
import jakarta.persistence.OptimisticLockException;
import jakarta.persistence.Persistence;
import java.io.InputStream;
import java.io.FileInputStream;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -106,7 +104,6 @@ public PolarisEclipseLinkMetaStoreSessionImpl(
properties.put(
JDBC_URL, properties.get(JDBC_URL).replace("{realm}", realmContext.getRealmIdentifier()));
}
properties.put(ECLIPSELINK_PERSISTENCE_XML, confFile);

emf = Persistence.createEntityManagerFactory(persistenceUnitName, properties);

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

try {
InputStream input = this.getClass().getClassLoader().getResourceAsStream(confFile);
FileInputStream input = new FileInputStream(confFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@guitcastro guitcastro Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aihuaxu aihuaxu Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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);

Copy link
Author

Choose a reason for hiding this comment

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

@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.

DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
DocumentBuilder builder = factory.newDocumentBuilder();
Document doc = builder.parse(input);
Expand All @@ -144,13 +141,13 @@ private Map<String, String> loadProperties(String confFile, String persistenceUn
this.properties = properties;
return properties;
} catch (Exception e) {
LOG.warn(
"Cannot find or parse the configuration file {} for persistence-unit {}",
confFile,
persistenceUnitName);
String errorMessage =
String.format(
"Cannot find or parse the configuration file {} for persistence-unit {}",
confFile,
persistenceUnitName);
throw new IllegalArgumentException(errorMessage, e);
}

return Maps.newHashMap();
}

/** {@inheritDoc} */
Expand Down
34 changes: 33 additions & 1 deletion ...a/com/snowflake/polaris/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,14 @@
import io.polaris.core.persistence.PolarisTestMetaStoreManager;
import io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkMetaStoreSessionImpl;
import io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkStore;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.ZoneId;
import java.util.Objects;
import java.util.UUID;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

/**
Expand All @@ -36,11 +43,20 @@ public class PolarisEclipseLinkMetaStoreTest extends PolarisMetaStoreManagerTest

@Override
protected PolarisTestMetaStoreManager createPolarisTestMetaStoreManager() {
Path tmpFile = Paths.get(String.format("/tmp/%s-persistence.xml", UUID.randomUUID()));
try {
Files.copy(
Objects.requireNonNull(getClass().getResourceAsStream("/META-INF/persistence.xml")),
tmpFile);
} catch (IOException e) {
throw new RuntimeException(e);
}

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return new PolarisTestMetaStoreManager(
new PolarisMetaStoreManagerImpl(),
new PolarisCallContext(
Expand All @@ -49,4 +65,20 @@ protected PolarisTestMetaStoreManager createPolarisTestMetaStoreManager() {
new PolarisConfigurationStore() {},
timeSource.withZone(ZoneId.systemDefault())));
}

@Test
void throwExceptionIfConfigFileDoesNotExists() {
PolarisDiagnostics diagServices = new PolarisDefaultDiagServiceImpl();
PolarisEclipseLinkStore store = new PolarisEclipseLinkStore(diagServices);
new PolarisEclipseLinkMetaStoreSessionImpl(
store, Mockito.mock(), () -> "realm", "does not exists", "polaris-dev");
}

@Test
void ensureNotLoadingDefaultClassPersistence() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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