You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "snazy (via GitHub)" <gi...@apache.org> on 2026/05/26 07:15:14 UTC
[PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
snazy opened a new pull request, #4544:
URL: https://github.com/apache/polaris/pull/4544
Stop sharing mutable privilege resolution state across Jackson mappers.
The previous setup leaked deserializer and serializer context between mappers and made tests depend on mapper construction order. Keep privilege resolution local to the module/mapper setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #4544:
URL: https://github.com/apache/polaris/pull/4544#discussion_r3315455494
##########
persistence/nosql/authz/impl/src/main/java/org/apache/polaris/persistence/nosql/authz/impl/PrivilegeSetDeserializer.java:
##########
@@ -18,38 +18,47 @@
*/
package org.apache.polaris.persistence.nosql.authz.impl;
-import static org.apache.polaris.persistence.nosql.authz.impl.JacksonPrivilegesModule.currentPrivileges;
+import static java.util.Objects.requireNonNull;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.JsonMappingException;
import java.io.IOException;
+import java.util.function.Supplier;
import org.apache.polaris.persistence.nosql.authz.api.PrivilegeSet;
+import org.apache.polaris.persistence.nosql.authz.api.Privileges;
class PrivilegeSetDeserializer extends JsonDeserializer<PrivilegeSet> {
+ private final Supplier<Privileges> privilegesResolver;
+
+ PrivilegeSetDeserializer(Supplier<Privileges> privilegesResolver) {
+ this.privilegesResolver =
+ requireNonNull(privilegesResolver, "privilegesResolver must not be null");
+ }
+
@Override
public PrivilegeSet deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
switch (p.currentToken()) {
case VALUE_NULL:
- return new PrivilegeSetImpl(currentPrivileges(), new byte[0]);
+ return new PrivilegeSetImpl(privilegesResolver.get(), new byte[0]);
case VALUE_STRING:
// Internal, storage serialization format.
var bytes = p.getBinaryValue();
- return new PrivilegeSetImpl(currentPrivileges(), bytes);
+ return new PrivilegeSetImpl(privilegesResolver.get(), bytes);
case START_ARRAY:
// External/REST serialization format using privilege names.
- var privileges = currentPrivileges();
+ var privileges = privilegesResolver.get();
var builder = PrivilegeSetImpl.builder(privileges);
for (var t = p.nextToken(); ; t = p.nextToken()) {
- // Note: switch(t) lets checkstyle fail
- if (t == JsonToken.VALUE_STRING) {
- builder.addPrivilege(privileges.byName(p.getText()));
- }
if (t == JsonToken.END_ARRAY) {
break;
}
+ if (t != JsonToken.VALUE_STRING) {
+ throw new JsonMappingException(p, "Unexpected JSON token " + t + " in privilege array");
Review Comment:
No behavior change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4544:
URL: https://github.com/apache/polaris/pull/4544#discussion_r3312666979
##########
persistence/nosql/authz/impl/src/main/java/org/apache/polaris/persistence/nosql/authz/impl/PrivilegeSetDeserializer.java:
##########
@@ -18,38 +18,47 @@
*/
package org.apache.polaris.persistence.nosql.authz.impl;
-import static org.apache.polaris.persistence.nosql.authz.impl.JacksonPrivilegesModule.currentPrivileges;
+import static java.util.Objects.requireNonNull;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.JsonMappingException;
import java.io.IOException;
+import java.util.function.Supplier;
import org.apache.polaris.persistence.nosql.authz.api.PrivilegeSet;
+import org.apache.polaris.persistence.nosql.authz.api.Privileges;
class PrivilegeSetDeserializer extends JsonDeserializer<PrivilegeSet> {
+ private final Supplier<Privileges> privilegesResolver;
+
+ PrivilegeSetDeserializer(Supplier<Privileges> privilegesResolver) {
+ this.privilegesResolver =
+ requireNonNull(privilegesResolver, "privilegesResolver must not be null");
+ }
+
@Override
public PrivilegeSet deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
switch (p.currentToken()) {
case VALUE_NULL:
- return new PrivilegeSetImpl(currentPrivileges(), new byte[0]);
+ return new PrivilegeSetImpl(privilegesResolver.get(), new byte[0]);
case VALUE_STRING:
// Internal, storage serialization format.
var bytes = p.getBinaryValue();
- return new PrivilegeSetImpl(currentPrivileges(), bytes);
+ return new PrivilegeSetImpl(privilegesResolver.get(), bytes);
case START_ARRAY:
// External/REST serialization format using privilege names.
- var privileges = currentPrivileges();
+ var privileges = privilegesResolver.get();
var builder = PrivilegeSetImpl.builder(privileges);
for (var t = p.nextToken(); ; t = p.nextToken()) {
- // Note: switch(t) lets checkstyle fail
- if (t == JsonToken.VALUE_STRING) {
- builder.addPrivilege(privileges.byName(p.getText()));
- }
if (t == JsonToken.END_ARRAY) {
break;
}
+ if (t != JsonToken.VALUE_STRING) {
+ throw new JsonMappingException(p, "Unexpected JSON token " + t + " in privilege array");
Review Comment:
the array path used to silently skip non-string tokens, now it throws. Good fix, is it a behavior change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4544:
URL: https://github.com/apache/polaris/pull/4544#discussion_r3303263327
##########
persistence/nosql/authz/impl/src/main/java/org/apache/polaris/persistence/nosql/authz/impl/JacksonPrivilegesModule.java:
##########
@@ -18,72 +18,71 @@
*/
package org.apache.polaris.persistence.nosql.authz.impl;
-import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
import com.fasterxml.jackson.databind.module.SimpleModule;
-import jakarta.enterprise.inject.Instance;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.event.Observes;
+import jakarta.enterprise.event.Shutdown;
+import jakarta.enterprise.event.Startup;
import jakarta.enterprise.inject.spi.CDI;
-import java.util.function.Function;
+import jakarta.inject.Inject;
+import java.util.function.Supplier;
import org.apache.polaris.persistence.nosql.authz.api.Acl;
import org.apache.polaris.persistence.nosql.authz.api.PrivilegeSet;
import org.apache.polaris.persistence.nosql.authz.api.Privileges;
public class JacksonPrivilegesModule extends SimpleModule {
+ @SuppressWarnings("unused")
public JacksonPrivilegesModule() {
- addDeserializer(PrivilegeSet.class, new PrivilegeSetDeserializer());
- addSerializer(PrivilegeSet.class, new PrivilegeSetSerializer());
- addDeserializer(Acl.class, new AclDeserializer());
- addSerializer(Acl.class, new AclSerializer());
+ this(PrivilegesViaCDI::privileges);
}
- static Privileges currentPrivileges() {
- return cdiResolve(Privileges.class);
+ JacksonPrivilegesModule(Supplier<Privileges> privilegesResolver) {
+ requireNonNull(privilegesResolver, "privilegesResolver must not be null");
+ addDeserializer(PrivilegeSet.class, new PrivilegeSetDeserializer(privilegesResolver));
+ addSerializer(PrivilegeSet.class, new PrivilegeSetSerializer(privilegesResolver));
+ addDeserializer(Acl.class, new AclDeserializer(privilegesResolver));
+ addSerializer(Acl.class, new AclSerializer());
}
- // TODO the following is the same as in AbstractTypeIdResolver
-
/**
- * Resolve the given type via {@link CDI#current() CDI.current()}. For tests the resolution via
- * CDI can be {@linkplain CDIResolver#setResolver(Function) routed to a custom function}.
+ * Helper to memoize the {@link Privileges} instance in a CDI environment. This class bridges the
+ * gap between CDI and Jackson module lifecycles.
+ *
+ * <p>The implementation attempts to infer the {@link Privileges} instance from the CDI container
+ * by observing the {@link Startup} event. However, other beans that also observe the {@link
+ * Startup} event can trigger a call to the {@link #privileges()} method before this bean's {@link
+ * #init(Startup)} function is called.
*/
- private static <R> R cdiResolve(Class<R> type) {
- // TODO instead of doing the 'CDIResolver' dance, we could (should?) have an attribute in the
- // `DatabindContext` holding a reference to the CDI instance (referred to as `Instance<?>`).
- var resolved = CDIResolver.resolver.apply(type);
- @SuppressWarnings("unchecked")
- var r = (R) resolved;
- return r;
- }
+ @ApplicationScoped
+ static class PrivilegesViaCDI {
Review Comment:
The static volatile field looks hacky. Isn't possible to stick to standard Quarkus ways to do this, e.g.
```java
@Singleton
class PrivilegesJacksonCustomizer implements ObjectMapperCustomizer {
@Inject Privileges privileges;
@Override
public void customize(ObjectMapper mapper) {
mapper.registerModule(new JacksonPrivilegesModule(() -> privileges));
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #4544:
URL: https://github.com/apache/polaris/pull/4544#discussion_r3303543915
##########
persistence/nosql/authz/impl/src/main/java/org/apache/polaris/persistence/nosql/authz/impl/JacksonPrivilegesModule.java:
##########
@@ -18,72 +18,71 @@
*/
package org.apache.polaris.persistence.nosql.authz.impl;
-import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
import com.fasterxml.jackson.databind.module.SimpleModule;
-import jakarta.enterprise.inject.Instance;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.event.Observes;
+import jakarta.enterprise.event.Shutdown;
+import jakarta.enterprise.event.Startup;
import jakarta.enterprise.inject.spi.CDI;
-import java.util.function.Function;
+import jakarta.inject.Inject;
+import java.util.function.Supplier;
import org.apache.polaris.persistence.nosql.authz.api.Acl;
import org.apache.polaris.persistence.nosql.authz.api.PrivilegeSet;
import org.apache.polaris.persistence.nosql.authz.api.Privileges;
public class JacksonPrivilegesModule extends SimpleModule {
+ @SuppressWarnings("unused")
public JacksonPrivilegesModule() {
- addDeserializer(PrivilegeSet.class, new PrivilegeSetDeserializer());
- addSerializer(PrivilegeSet.class, new PrivilegeSetSerializer());
- addDeserializer(Acl.class, new AclDeserializer());
- addSerializer(Acl.class, new AclSerializer());
+ this(PrivilegesViaCDI::privileges);
}
- static Privileges currentPrivileges() {
- return cdiResolve(Privileges.class);
+ JacksonPrivilegesModule(Supplier<Privileges> privilegesResolver) {
+ requireNonNull(privilegesResolver, "privilegesResolver must not be null");
+ addDeserializer(PrivilegeSet.class, new PrivilegeSetDeserializer(privilegesResolver));
+ addSerializer(PrivilegeSet.class, new PrivilegeSetSerializer(privilegesResolver));
+ addDeserializer(Acl.class, new AclDeserializer(privilegesResolver));
+ addSerializer(Acl.class, new AclSerializer());
}
- // TODO the following is the same as in AbstractTypeIdResolver
-
/**
- * Resolve the given type via {@link CDI#current() CDI.current()}. For tests the resolution via
- * CDI can be {@linkplain CDIResolver#setResolver(Function) routed to a custom function}.
+ * Helper to memoize the {@link Privileges} instance in a CDI environment. This class bridges the
+ * gap between CDI and Jackson module lifecycles.
+ *
+ * <p>The implementation attempts to infer the {@link Privileges} instance from the CDI container
+ * by observing the {@link Startup} event. However, other beans that also observe the {@link
+ * Startup} event can trigger a call to the {@link #privileges()} method before this bean's {@link
+ * #init(Startup)} function is called.
*/
- private static <R> R cdiResolve(Class<R> type) {
- // TODO instead of doing the 'CDIResolver' dance, we could (should?) have an attribute in the
- // `DatabindContext` holding a reference to the CDI instance (referred to as `Instance<?>`).
- var resolved = CDIResolver.resolver.apply(type);
- @SuppressWarnings("unchecked")
- var r = (R) resolved;
- return r;
- }
+ @ApplicationScoped
+ static class PrivilegesViaCDI {
Review Comment:
Ideally, yes. But Weld doesn't support `@Singleton`. :shrug:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NoSQL: Scope authz Jackson privilege resolution to each mapper [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy merged PR #4544:
URL: https://github.com/apache/polaris/pull/4544
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org