Skip to content

Make CallContextCatalogFactory request-scoped #2972

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make CallContextCatalogFactory request-scoped #2972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions ...rvice/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,7 @@ protected void initializeCatalog() {
this.baseCatalog = federatedCatalog;
} else {
LOGGER.atInfo().log("Initializing non-federated catalog");
this.baseCatalog =
catalogFactory.createCallContextCatalog(
callContext, polarisPrincipal, resolutionManifest);
this.baseCatalog = catalogFactory.createCallContextCatalog(resolutionManifest);
}
this.namespaceCatalog =
(baseCatalog instanceof SupportsNamespaces) ? (SupportsNamespaces) baseCatalog : null;
Expand Down
7 changes: 1 addition & 6 deletions ...e/src/main/java/org/apache/polaris/service/context/catalog/CallContextCatalogFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,9 @@
package org.apache.polaris.service.context.catalog;

import org.apache.iceberg.catalog.Catalog;
import org.apache.polaris.core.auth.PolarisPrincipal;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;

public interface CallContextCatalogFactory {

Catalog createCallContextCatalog(
CallContext context,
PolarisPrincipal polarisPrincipal,
PolarisResolutionManifest resolvedManifest);
Catalog createCallContextCatalog(PolarisResolutionManifest resolvedManifest);
}
33 changes: 18 additions & 15 deletions ...ain/java/org/apache/polaris/service/context/catalog/PolarisCallContextCatalogFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
package org.apache.polaris.service.context.catalog;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.context.RequestScoped;
import jakarta.inject.Inject;
import java.util.HashMap;
import java.util.Map;
Expand All @@ -28,7 +28,7 @@
import org.apache.polaris.core.auth.PolarisPrincipal;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.CatalogEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.core.persistence.resolver.ResolverFactory;
import org.apache.polaris.service.catalog.iceberg.IcebergCatalog;
Expand All @@ -39,7 +39,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@ApplicationScoped
@RequestScoped
Copy link
Contributor

@adutra adutra Nov 5, 2025

Choose a reason for hiding this comment

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

A general remark: just because a CDI bean is being injected request-scoped beans, doesn't mean the bean itself must switch to request scope.

All is needed is that the request scope must be active when any of the injected request-scoped beans is accessed.

Here for instance, this is the case, so keeping @ApplicationScoped doesn't hurt. I tried, and all tests passed.

Keeping application scope whenever possible reduces the number of proxies created for each request.

Copy link
Contributor Author

@XN137 XN137 Nov 5, 2025

Choose a reason for hiding this comment

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

i thought we generally wanted to avoid injecting request-scoped beans into application-scoped beans...
so whats the guiding principle when something should be application or request scoped then for you?

i thought since the PolarisPrincipal is being derived from SecurityContext and is now a ctor parameter, we should mark this class as request-scoped, to make clear this factory is only usable while handling a request.
and as stated in the PR description afaict the factory is only ever injected into IcebergCatalogAdapter which is marked as request-scoped already.

Copy link
Contributor

@dimas-b dimas-b Nov 5, 2025

Choose a reason for hiding this comment

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

Injecting request-scoped PolarisPrincipal into an @ApplicationScoped bean will work in Quarkus, but the injected object will be a proxy, switching to the active context in runtime.

From my POV making this bean @RequestScoped is nicer because it makes bean grouping more explicit and this bean does not have to keep any global state.

public class PolarisCallContextCatalogFactory implements CallContextCatalogFactory {
private static final Logger LOGGER =
LoggerFactory.getLogger(PolarisCallContextCatalogFactory.class);
Expand All @@ -49,47 +49,50 @@ public class PolarisCallContextCatalogFactory implements CallContextCatalogFacto
private final AccessConfigProvider accessConfigProvider;
private final FileIOFactory fileIOFactory;
private final ResolverFactory resolverFactory;
private final MetaStoreManagerFactory metaStoreManagerFactory;
private final PolarisEventListener polarisEventListener;
private final PolarisMetaStoreManager metaStoreManager;
private final CallContext callContext;
private final PolarisPrincipal principal;

@Inject
public PolarisCallContextCatalogFactory(
PolarisDiagnostics diagnostics,
ResolverFactory resolverFactory,
MetaStoreManagerFactory metaStoreManagerFactory,
TaskExecutor taskExecutor,
AccessConfigProvider accessConfigProvider,
FileIOFactory fileIOFactory,
PolarisEventListener polarisEventListener) {
PolarisEventListener polarisEventListener,
PolarisMetaStoreManager metaStoreManager,
CallContext callContext,
PolarisPrincipal principal) {
this.diagnostics = diagnostics;
this.resolverFactory = resolverFactory;
this.metaStoreManagerFactory = metaStoreManagerFactory;
this.taskExecutor = taskExecutor;
this.accessConfigProvider = accessConfigProvider;
this.fileIOFactory = fileIOFactory;
this.polarisEventListener = polarisEventListener;
this.metaStoreManager = metaStoreManager;
this.callContext = callContext;
this.principal = principal;
}

@Override
public Catalog createCallContextCatalog(
CallContext context,
PolarisPrincipal polarisPrincipal,
final PolarisResolutionManifest resolvedManifest) {
public Catalog createCallContextCatalog(final PolarisResolutionManifest resolvedManifest) {
CatalogEntity catalog = resolvedManifest.getResolvedCatalogEntity();
String catalogName = catalog.getName();

String realm = context.getRealmContext().getRealmIdentifier();
String realm = callContext.getRealmContext().getRealmIdentifier();
String catalogKey = realm + "/" + catalogName;
LOGGER.debug("Initializing new BasePolarisCatalog for key: {}", catalogKey);

IcebergCatalog catalogInstance =
new IcebergCatalog(
diagnostics,
resolverFactory,
metaStoreManagerFactory.getOrCreateMetaStoreManager(context.getRealmContext()),
context,
metaStoreManager,
callContext,
resolvedManifest,
polarisPrincipal,
principal,
taskExecutor,
accessConfigProvider,
fileIOFactory,
Expand Down
22 changes: 12 additions & 10 deletions runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ public void before(TestInfo testInfo) {
PrincipalEntity rootPrincipal =
metaStoreManager.findRootPrincipal(polarisContext).orElseThrow();
this.authenticatedRoot = PolarisPrincipal.of(rootPrincipal, Set.of());
QuarkusMock.installMockForType(authenticatedRoot, PolarisPrincipal.class);

this.adminService =
new PolarisAdminService(
Expand Down Expand Up @@ -518,36 +519,37 @@ public static class TestPolarisCallContextCatalogFactory

@SuppressWarnings("unused") // Required by CDI
protected TestPolarisCallContextCatalogFactory() {
this(null, null, null, null, null, null, null);
this(null, null, null, null, null, null, null, null, null);
}

@Inject
public TestPolarisCallContextCatalogFactory(
PolarisDiagnostics diagnostics,
ResolverFactory resolverFactory,
MetaStoreManagerFactory metaStoreManagerFactory,
TaskExecutor taskExecutor,
AccessConfigProvider accessConfigProvider,
FileIOFactory fileIOFactory,
PolarisEventListener polarisEventListener) {
PolarisEventListener polarisEventListener,
PolarisMetaStoreManager metaStoreManager,
CallContext callContext,
PolarisPrincipal principal) {
super(
diagnostics,
resolverFactory,
metaStoreManagerFactory,
taskExecutor,
accessConfigProvider,
fileIOFactory,
polarisEventListener);
polarisEventListener,
metaStoreManager,
callContext,
principal);
}

@Override
public Catalog createCallContextCatalog(
CallContext context,
PolarisPrincipal polarisPrincipal,
final PolarisResolutionManifest resolvedManifest) {
public Catalog createCallContextCatalog(PolarisResolutionManifest resolvedManifest) {
// This depends on the BasePolarisCatalog allowing calling initialize multiple times
// to override the previous config.
Catalog catalog = super.createCallContextCatalog(context, polarisPrincipal, resolvedManifest);
Catalog catalog = super.createCallContextCatalog(resolvedManifest);
catalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand Down
17 changes: 7 additions & 10 deletions .../test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerAuthzTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,8 +1124,7 @@ private IcebergCatalogHandler newWrapperWithFineGrainedAuthzDisabled() {
CallContextCatalogFactory mockFactory = Mockito.mock(CallContextCatalogFactory.class);

// Mock the catalog factory to return our regular catalog but with mocked config
Mockito.when(mockFactory.createCallContextCatalog(Mockito.any(), Mockito.any(), Mockito.any()))
.thenReturn(baseCatalog);
Mockito.when(mockFactory.createCallContextCatalog(Mockito.any())).thenReturn(baseCatalog);

return newWrapperWithFineLevelAuthDisabled(Set.of(), CATALOG_NAME, mockFactory, false);
}
Expand Down Expand Up @@ -1894,18 +1893,16 @@ public void testSendNotificationSufficientPrivileges() {
new PolarisCallContextCatalogFactory(
diagServices,
resolverFactory,
managerFactory,
Mockito.mock(),
accessConfigProvider,
new DefaultFileIOFactory(),
polarisEventListener) {
polarisEventListener,
metaStoreManager,
callContext,
authenticatedRoot) {
@Override
public Catalog createCallContextCatalog(
CallContext context,
PolarisPrincipal polarisPrincipal,
PolarisResolutionManifest resolvedManifest) {
Catalog catalog =
super.createCallContextCatalog(context, polarisPrincipal, resolvedManifest);
public Catalog createCallContextCatalog(PolarisResolutionManifest resolvedManifest) {
Catalog catalog = super.createCallContextCatalog(resolvedManifest);
String fileIoImpl = "org.apache.iceberg.inmemory.InMemoryFileIO";
catalog.initialize(
externalCatalog, ImmutableMap.of(CatalogProperties.FILE_IO_IMPL, fileIoImpl));
Expand Down
72 changes: 37 additions & 35 deletions runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,39 @@ public TestServices build() {
PolarisMetaStoreManager metaStoreManager =
metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext);

CreatePrincipalResult createdPrincipal =
metaStoreManager.createPrincipal(
callContext.getPolarisCallContext(),
new PrincipalEntity.Builder()
.setName("test-principal")
.setCreateTimestamp(Instant.now().toEpochMilli())
.setCredentialRotationRequiredState()
.build());
PolarisPrincipal principal = PolarisPrincipal.of(createdPrincipal.getPrincipal(), Set.of());

SecurityContext securityContext =
new SecurityContext() {
@Override
public Principal getUserPrincipal() {
return principal;
}

@Override
public boolean isUserInRole(String s) {
return false;
}

@Override
public boolean isSecure() {
return true;
}

@Override
public String getAuthenticationScheme() {
return "";
}
};

EntityCache entityCache =
metaStoreManagerFactory.getOrCreateEntityCache(realmContext, realmConfig);
ResolverFactory resolverFactory =
Expand Down Expand Up @@ -250,11 +283,13 @@ public TestServices build() {
new PolarisCallContextCatalogFactory(
diagnostics,
resolverFactory,
metaStoreManagerFactory,
taskExecutor,
accessConfigProvider,
fileIOFactory,
polarisEventListener);
polarisEventListener,
metaStoreManager,
callContext,
principal);

ReservedProperties reservedProperties = ReservedProperties.NONE;

Expand Down Expand Up @@ -287,39 +322,6 @@ public TestServices build() {
IcebergRestConfigurationApi restConfigurationApi =
new IcebergRestConfigurationApi(catalogService);

CreatePrincipalResult createdPrincipal =
metaStoreManager.createPrincipal(
callContext.getPolarisCallContext(),
new PrincipalEntity.Builder()
.setName("test-principal")
.setCreateTimestamp(Instant.now().toEpochMilli())
.setCredentialRotationRequiredState()
.build());
PolarisPrincipal principal = PolarisPrincipal.of(createdPrincipal.getPrincipal(), Set.of());

SecurityContext securityContext =
new SecurityContext() {
@Override
public Principal getUserPrincipal() {
return principal;
}

@Override
public boolean isUserInRole(String s) {
return false;
}

@Override
public boolean isSecure() {
return true;
}

@Override
public String getAuthenticationScheme() {
return "";
}
};

PolarisAdminService adminService =
new PolarisAdminService(
callContext,
Expand Down