Skip to content

Add type-check to PolarisEntity subclass ctors #2302

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

Merged
merged 1 commit into from
Sep 2, 2025
Merged

Add type-check to PolarisEntity subclass ctors #2302

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
5 changes: 4 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.apache.polaris.core.admin.model.StorageConfigInfo.StorageTypeEnum.AZURE;

import com.google.common.base.Preconditions;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.Collection;
Expand Down Expand Up @@ -70,9 +71,11 @@ public class CatalogEntity extends PolarisEntity implements LocationBasedEntity

public CatalogEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.CATALOG, "Invalid entity type: %s", getType());
}

public static CatalogEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable CatalogEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new CatalogEntity(sourceEntity);
}
Expand Down
6 changes: 5 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogRoleEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@
*/
package org.apache.polaris.core.entity;

import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.admin.model.CatalogRole;

/** Wrapper for translating between the REST CatalogRole object and the base PolarisEntity type. */
public class CatalogRoleEntity extends PolarisEntity {
public CatalogRoleEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.CATALOG_ROLE, "Invalid entity type: %s", getType());
}

public static CatalogRoleEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable CatalogRoleEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new CatalogRoleEntity(sourceEntity);
}
Expand Down
6 changes: 5 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/NamespaceEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package org.apache.polaris.core.entity;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.rest.RESTUtil;
import org.apache.polaris.core.catalog.PolarisCatalogHelpers;
Expand All @@ -33,9 +35,11 @@ public class NamespaceEntity extends PolarisEntity implements LocationBasedEntit

public NamespaceEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.NAMESPACE, "Invalid entity type: %s", getType());
}

public static NamespaceEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable NamespaceEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new NamespaceEntity(sourceEntity);
}
Expand Down
5 changes: 3 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -151,14 +152,14 @@ public PolarisEntity(
entityVersion);
}

public static PolarisEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable PolarisEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new PolarisEntity(sourceEntity);
}
return null;
}

public static PolarisEntity of(EntityResult result) {
public static @Nullable PolarisEntity of(EntityResult result) {
if (result.isSuccess()) {
return new PolarisEntity(result.getEntity());
}
Expand Down
6 changes: 5 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@
*/
package org.apache.polaris.core.entity;

import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.admin.model.Principal;

/** Wrapper for translating between the REST Principal object and the base PolarisEntity type. */
public class PrincipalEntity extends PolarisEntity {
public PrincipalEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.PRINCIPAL, "Invalid entity type: %s", getType());
}

public static PrincipalEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable PrincipalEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new PrincipalEntity(sourceEntity);
}
Expand Down
6 changes: 5 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.polaris.core.entity;

import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.admin.model.PrincipalRole;
import org.apache.polaris.core.entity.table.federated.FederatedEntities;

Expand All @@ -27,9 +29,11 @@
public class PrincipalRoleEntity extends PolarisEntity {
public PrincipalRoleEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.PRINCIPAL_ROLE, "Invalid entity type: %s", getType());
}

public static PrincipalRoleEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable PrincipalRoleEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new PrincipalRoleEntity(sourceEntity);
}
Expand Down
13 changes: 8 additions & 5 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/TaskEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.polaris.core.entity;

import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.polaris.core.persistence.PolarisObjectMapperUtil;

/**
Expand All @@ -27,14 +29,15 @@
public class TaskEntity extends PolarisEntity {
public TaskEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.TASK, "Invalid entity type: %s", getType());
}

public static TaskEntity of(PolarisBaseEntity polarisEntity) {
if (polarisEntity != null) {
return new TaskEntity(polarisEntity);
} else {
return null;
public static @Nullable TaskEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new TaskEntity(sourceEntity);
}
return null;
}

public <T> T readData(Class<T> klass) {
Expand Down
3 changes: 2 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/table/GenericTableEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.polaris.core.entity.table;

import com.fasterxml.jackson.annotation.JsonIgnore;
import jakarta.annotation.Nullable;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.RESTUtil;
Expand All @@ -42,7 +43,7 @@ public GenericTableEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
}

public static GenericTableEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable GenericTableEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new GenericTableEntity(sourceEntity);
}
Expand Down
3 changes: 2 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/entity/table/IcebergTableLikeEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.polaris.core.entity.table;

import com.fasterxml.jackson.annotation.JsonIgnore;
import jakarta.annotation.Nullable;
import java.util.Optional;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
Expand Down Expand Up @@ -48,7 +49,7 @@ public IcebergTableLikeEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
}

public static IcebergTableLikeEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable IcebergTableLikeEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new IcebergTableLikeEntity(sourceEntity);
}
Expand Down
3 changes: 3 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/table/TableLikeEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.polaris.core.entity.table;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.base.Preconditions;
import jakarta.annotation.Nonnull;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
Expand All @@ -37,6 +38,8 @@ public abstract class TableLikeEntity extends PolarisEntity implements LocationB

public TableLikeEntity(@Nonnull PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.TABLE_LIKE, "Invalid entity type: %s", getType());
}

@JsonIgnore
Expand Down
6 changes: 4 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/policy/PolicyEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.google.common.base.Preconditions;
import jakarta.annotation.Nullable;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.rest.RESTUtil;
import org.apache.polaris.core.entity.NamespaceEntity;
Expand All @@ -36,13 +37,14 @@ public class PolicyEntity extends PolarisEntity {

PolicyEntity(PolarisBaseEntity sourceEntity) {
super(sourceEntity);
Preconditions.checkState(
getType() == PolarisEntityType.POLICY, "Invalid entity type: %s", getType());
}

public static PolicyEntity of(PolarisBaseEntity sourceEntity) {
public static @Nullable PolicyEntity of(@Nullable PolarisBaseEntity sourceEntity) {
if (sourceEntity != null) {
return new PolicyEntity(sourceEntity);
}

return null;
}

Expand Down
1 change: 1 addition & 0 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,7 @@ private void validateNoLocationOverlap(
IcebergTableLikeEntity virtualEntity =
IcebergTableLikeEntity.of(
new PolarisEntity.Builder()
.setType(PolarisEntityType.TABLE_LIKE)
.setParentId(resolvedNamespace.getLast().getId())
.setProperties(Map.of(PolarisEntityConstants.ENTITY_BASE_LOCATION, location))
.build());
Expand Down
14 changes: 10 additions & 4 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 @@ -24,6 +24,7 @@
import com.google.common.collect.Maps;
import io.smallrye.common.annotation.Identifier;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.enterprise.inject.Instance;
import jakarta.ws.rs.core.SecurityContext;
import java.io.Closeable;
Expand Down Expand Up @@ -81,7 +82,9 @@
import org.apache.polaris.core.connection.ConnectionType;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.CatalogEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntitySubType;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.table.IcebergTableLikeEntity;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
Expand Down Expand Up @@ -579,12 +582,15 @@ public boolean sendNotification(TableIdentifier identifier, NotificationRequest
* Fetch the metastore table entity for the given table identifier
*
* @param tableIdentifier The identifier of the table
* @return the Polaris table entity for the table
* @return the Polaris table entity for the table or null for external catalogs
*/
private IcebergTableLikeEntity getTableEntity(TableIdentifier tableIdentifier) {
private @Nullable IcebergTableLikeEntity getTableEntity(TableIdentifier tableIdentifier) {
PolarisResolvedPathWrapper target = resolutionManifest.getResolvedPath(tableIdentifier);

return IcebergTableLikeEntity.of(target.getRawLeafEntity());
PolarisEntity rawLeafEntity = target.getRawLeafEntity();
if (rawLeafEntity.getType() == PolarisEntityType.TABLE_LIKE) {
return IcebergTableLikeEntity.of(rawLeafEntity);
}
return null; // could be an external catalog
Copy link
Contributor Author

@XN137 XN137 Aug 25, 2025

Choose a reason for hiding this comment

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

without this change the CatalogFederationIT was failing:

getTableEntity: ns1.test_table -> resolvedPath:[entity:name=test_catalog_external;id=6;parentId=0;entityVersion=1;type=CATALOG;subType=NULL_SUBTYPE;internalProperties={catalogType=EXTERNAL, storage_configuration_info={"@type":"FileStorageConfigurationInfo","allowedLocations":["file:///tmp/warehouse"],"storageType":"FILE","fileIoImplClassName":"org.apache.iceberg.hadoop.HadoopFileIO"}, connection_configuration_info={"uri":"http://localhost:42705/api/catalog","authenticationParameters":{"tokenUri":"http://localhost:42705/api/catalog/v1/oauth/tokens","clientId":"71667f9b548ce462","clientSecretReference":{"urn":"urn:polaris-secret:unsafe-in-memory:6:0","referencePayload":{"ciphertext-hash":"adf96dbbdf0d7b85a4940ff1e5bd912526fab136e9b66896efcd641be8bd3c6c","encryption-key":"hDB047wH0cue+kMm6v28HhJI9cK6JOs63SWqzR39V+o="}},"scopes":["PRINCIPAL_ROLE:ALL"],"authenticationTypeCode":1},"remoteCatalogName":"test_catalog_local","connectionTypeCode":1}};grantRecordsAsGrantee:[];grantRecordsAsSecurable:[PolarisGrantRec{securableCatalogId=0, securableId=6, granteeCatalogId=6, granteeId=7, privilegeCode=2}, PolarisGrantRec{securableCatalogId=0, securableId=6, granteeCatalogId=6, granteeId=7, privilegeCode=21}, PolarisGrantRec{securableCatalogId=0, securableId=6, granteeCatalogId=6, granteeId=7, privilegeCode=31}]] -> name=test_catalog_external;id=6;parentId=0;entityVersion=1;type=CATALOG;subType=NULL_SUBTYPE;internalProperties={catalogType=EXTERNAL, storage_configuration_info={"@type":"FileStorageConfigurationInfo","allowedLocations":["file:///tmp/warehouse"],"storageType":"FILE","fileIoImplClassName":"org.apache.iceberg.hadoop.HadoopFileIO"}, connection_configuration_info={"uri":"http://localhost:42705/api/catalog","authenticationParameters":{"tokenUri":"http://localhost:42705/api/catalog/v1/oauth/tokens","clientId":"71667f9b548ce462","clientSecretReference":{"urn":"urn:polaris-secret:unsafe-in-memory:6:0","referencePayload":{"ciphertext-hash":"adf96dbbdf0d7b85a4940ff1e5bd912526fab136e9b66896efcd641be8bd3c6c","encryption-key":"hDB047wH0cue+kMm6v28HhJI9cK6JOs63SWqzR39V+o="}},"scopes":["PRINCIPAL_ROLE:ALL"],"authenticationTypeCode":1},"remoteCatalogName":"test_catalog_local","connectionTypeCode":1}}
2025-08-25 08:43:48,888 INFO  [org.apa.pol.ser.exc.IcebergExceptionMapper] [,POLARIS] [,,,] (executor-thread-1) Handling runtimeException Invalid entity type: CATALOG
2025-08-25 08:43:48,888 INFO  [org.apa.pol.ser.exc.IcebergExceptionMapper] [,POLARIS] [,,,] (executor-thread-1) Full RuntimeException: java.lang.IllegalStateException: Invalid entity type: CATALOG
	at com.google.common.base.Preconditions.checkState(Preconditions.java:603)
	at org.apache.polaris.core.entity.table.TableLikeEntity.<init>(TableLikeEntity.java:41)
	at org.apache.polaris.core.entity.table.IcebergTableLikeEntity.<init>(IcebergTableLikeEntity.java:49)
	at org.apache.polaris.core.entity.table.IcebergTableLikeEntity.of(IcebergTableLikeEntity.java:54)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogHandler.getTableEntity(IcebergCatalogHandler.java:578)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogHandler.loadTableIfStale(IcebergCatalogHandler.java:603)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogAdapter.lambda$loadTable$10(IcebergCatalogAdapter.java:431)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogAdapter.withCatalog(IcebergCatalogAdapter.java:193)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogAdapter.loadTable(IcebergCatalogAdapter.java:422)
	at org.apache.polaris.service.catalog.iceberg.IcebergCatalogAdapter_ClientProxy.loadTable(Unknown Source)

Copy link
Contributor

@dimas-b dimas-b Aug 26, 2025

Choose a reason for hiding this comment

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

I wonder how the entity path ends up pointing to a CATALOG when it's a loadTable call 🤔

Copy link
Contributor Author

@XN137 XN137 Aug 27, 2025

Choose a reason for hiding this comment

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

idk how external catalog integration works under the hood but in the first output line (that i had added) we can see that the PolarisResolvedPathWrapper ends up pointing to the external catalog entity (which is also the leaf element).

so i assumed for external catalogs in general we dont have any actual entity representing the table? but maybe this is just an artifact of an incorrectly written test idk.

the return value of getTableEntity is only used for supporting ETags not for loading the table:

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

Lines 601 to 619 in 3f1dfb3

if (ifNoneMatch != null) {
// Perform freshness-aware table loading if caller specified ifNoneMatch.
IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
if (tableEntity == null || tableEntity.getMetadataLocation() == null) {
LOGGER
.atWarn()
.addKeyValue("tableIdentifier", tableIdentifier)
.addKeyValue("tableEntity", tableEntity)
.log("Failed to getMetadataLocation to generate ETag when loading table");
} else {
// TODO: Refactor null-checking into the helper method once we create a more canonical
// interface for associate etags with entities.
String tableEntityTag =
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
if (ifNoneMatch.anyMatch(tableEntityTag)) {
return Optional.empty();
}
}
}

Copy link
Contributor

@dimas-b dimas-b Aug 27, 2025

Choose a reason for hiding this comment

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

@dennishuo : do you have any insight to share?

Copy link
Contributor

@dimas-b dimas-b Aug 27, 2025

Choose a reason for hiding this comment

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

to be clear: this conversation is interesting, but the changes in this PR still make sense from my POV 🙂

}

public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) {
Expand Down