This is an automated email from the ASF dual-hosted git repository. Cole-Greer pushed a commit to branch PDT in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit c239ee6a2f6d3d85fd71323cacb481d450bd9b2e Author: Cole Greer <[email protected]> AuthorDate: Wed Jun 10 20:05:15 2026 -0700 Address PDT PR review: validation, docs, precedence tests, cleanup - Python: enforce String-only keys on ProviderDefinedType property maps to match the other GLVs, with a unit test. - Docs: document the non-empty type name and String property-key constraints inline in the provider PDT section (covering all GLVs); add a JPMS note explaining annotation-based hydration needs the provider module to open its package to gremlin-core; integrate the CustomTypeSerializer breaking-change migration guidance into the existing 4.x upgrade entry. - All GLVs: add a maintenance comment and a lightweight test verifying that a registered adapter takes precedence over the @ProviderDefined annotation during dehydration (Java, Python, Go, .NET). - Remove the now-orphaned GraphBinaryIo class (all references were removed earlier in this PR). - Remove the unused maven-jar-plugin test-jar execution from gremlin-server. Assisted-by: Kiro:claude-opus-4.8 --- docs/src/dev/provider/index.asciidoc | 38 ++++++++++++-- docs/src/upgrade/release-4.x.x.asciidoc | 15 +++--- .../gremlin/process/traversal/GremlinLang.java | 2 + .../gremlin/structure/io/binary/GraphBinaryIo.java | 60 ---------------------- .../gremlin/process/traversal/GremlinLangTest.java | 42 +++++++++++++++ .../Gremlin.Net/Process/Traversal/GremlinLang.cs | 3 +- .../Process/Traversal/GremlinLangTests.cs | 35 +++++++++++++ gremlin-go/driver/gremlinlang.go | 4 +- gremlin-go/driver/gremlinlang_test.go | 27 ++++++++++ .../python/gremlin_python/process/traversal.py | 4 +- .../main/python/gremlin_python/structure/graph.py | 2 + .../python/tests/unit/process/test_gremlin_lang.py | 22 ++++++++ .../structure/io/test_provider_defined_type.py | 4 ++ gremlin-server/pom.xml | 10 ---- 14 files changed, 185 insertions(+), 83 deletions(-) diff --git a/docs/src/dev/provider/index.asciidoc b/docs/src/dev/provider/index.asciidoc index 7de043bc9d..511e6a67cd 100644 --- a/docs/src/dev/provider/index.asciidoc +++ b/docs/src/dev/provider/index.asciidoc @@ -1362,10 +1362,11 @@ public class Point { } ---- -The `name` attribute is a unique identifier for the type. It is strongly recommended to namespace type names using -your graph's identifier as a prefix (e.g. `"mygraph:Point"`). This avoids collisions when clients interact with -multiple providers and makes the origin of a type immediately clear. By default, all fields are included. Use -`includedFields` or `excludedFields` to control which fields are serialized: +The `name` attribute is a unique identifier for the type. It must not be null or empty — all GLVs reject an +empty name when a type is defined or a `ProviderDefinedType` is constructed. It is strongly recommended to namespace +type names using your graph's identifier as a prefix (e.g. `"mygraph:Point"`). This avoids collisions when clients +interact with multiple providers and makes the origin of a type immediately clear. By default, all fields are included. +Use `includedFields` or `excludedFields` to control which fields are serialized: [source,java] ---- @@ -1382,6 +1383,10 @@ constructor and the mapped fields must be directly settable (e.g. public fields) requirements — for example those with immutable `final` fields or no default constructor — should instead use a `ProviderDefinedTypeAdapter` (see <<adapter-for-types-you-don-t-own>>), which gives full control over construction. +The serialized field set becomes a property map whose keys are always strings. In statically typed GLVs (Java, .NET, +Go, and TypeScript) this is enforced by the type system; in Python the keys are validated at runtime and a +`TypeError` is raised for any non-string key. + ==== Nested Types PDT supports nested custom types. Each nested type must also be annotated: @@ -1541,6 +1546,31 @@ serialization and the registry handles inbound reconstruction. For driver users consuming PDTs, see the <<gremlin-variants,Gremlin Variants>> reference documentation for each language driver. +==== JPMS Considerations + +Annotation-based PDT hydration uses reflection internally — specifically `Constructor.setAccessible(true)` and +`Field.set()` on the provider's annotated class (see `AnnotatedTypeAdapter.fromProperties` in +`ProviderDefinedTypeRegistry`). Under the Java Platform Module System (JPMS), this means the provider's module must +`opens` its PDT package to `org.apache.tinkerpop.gremlin.core` (or to `ALL-UNNAMED` for classpath usage), otherwise +the JVM will throw `InaccessibleObjectException` at hydration time. + +For example, in the provider's `module-info.java`: + +[source,java] +---- +opens com.example.graph.types to org.apache.tinkerpop.gremlin.core; +---- + +Alternatively, providers that cannot modify their module descriptor can pass JVM flags: + +[source,text] +---- +--add-opens=com.example.graph/com.example.graph.types=ALL-UNNAMED +---- + +NOTE: This only applies to annotation-based hydration. Providers using `ProviderDefinedTypeAdapter` with explicit +`fromProperties` logic do not require reflective access and are unaffected by JPMS restrictions. + [[gremlin-plugins]] == Gremlin Plugins diff --git a/docs/src/upgrade/release-4.x.x.asciidoc b/docs/src/upgrade/release-4.x.x.asciidoc index e0cd2e72a6..1de2d34f78 100644 --- a/docs/src/upgrade/release-4.x.x.asciidoc +++ b/docs/src/upgrade/release-4.x.x.asciidoc @@ -510,12 +510,15 @@ effort to the old custom serializer approach but is entirely optional for basic ===== Provider Defined Types -TinkerPop 4 replaces the TP3 `CustomTypeSerializer` mechanism with Provider Defined Types (PDT). The key improvement -is that driver users receive PDT values as structured `ProviderDefinedType` objects by default, without any -configuration — eliminating the serializer errors that unknown custom types caused in TP3. Providers expose types by -annotating classes with `@ProviderDefined` or implementing `ProviderDefinedTypeAdapter<T>` for types they don't own; -adapters are discovered automatically via ServiceLoader, requiring similar effort to the old approach but benefiting -all driver users transparently. +TinkerPop 4 replaces the TP3 `CustomTypeSerializer` mechanism with Provider Defined Types (PDT). The TP3 extension +point has been removed entirely and there is no backward-compatible bridge, so any existing `CustomTypeSerializer` +implementations must be migrated: providers expose their custom types by annotating classes with `@ProviderDefined` +or, for types they don't own, by implementing `ProviderDefinedTypeAdapter<T>`. Adapters are discovered automatically +via ServiceLoader, so this requires similar effort to the old approach. + +The key improvement is that driver users now receive PDT values as structured `ProviderDefinedType` objects by +default, without any configuration — eliminating the serializer errors that unknown custom types caused in TP3, and +benefiting all driver users transparently. See <<provider-defined-types>> for full details on annotation usage, field filtering, nested types, and ServiceLoader registration. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java index 14d153e7ee..3e651be971 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLang.java @@ -267,6 +267,8 @@ public class GremlinLang implements Cloneable, Serializable { return ((Class) arg).getSimpleName(); } + // Intentional precedence: a registered adapter takes priority over @ProviderDefined annotation + // so that providers/users can override annotation-derived behavior with an explicit adapter. if (pdtRegistry != null) { final Optional<ProviderDefinedTypeAdapter<?>> adapter = pdtRegistry.getAdapterByClass(arg.getClass()); if (adapter.isPresent()) { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/GraphBinaryIo.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/GraphBinaryIo.java deleted file mode 100644 index 8a1e2c0e4e..0000000000 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/GraphBinaryIo.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.tinkerpop.gremlin.structure.io.binary; - -import org.apache.tinkerpop.gremlin.structure.io.GraphReader; -import org.apache.tinkerpop.gremlin.structure.io.GraphWriter; -import org.apache.tinkerpop.gremlin.structure.io.Io; -import org.apache.tinkerpop.gremlin.structure.io.IoRegistry; -import org.apache.tinkerpop.gremlin.structure.io.Mapper; - -import java.io.IOException; - -/** - * This is a dummy implementation of {@link Io} which is only used in the context of helping to configure a - * GraphBinary {@code MessageSerializer} with an {@link IoRegistry}. It's methods are not implemented. - * - * @author Stephen Mallette (http://stephen.genoprime.com) - */ -public class GraphBinaryIo implements Io { - @Override - public GraphReader.ReaderBuilder reader() { - throw new UnsupportedOperationException("GraphBinaryIo is only used to support IoRegistry configuration - it's methods are not implemented"); - } - - @Override - public GraphWriter.WriterBuilder writer() { - throw new UnsupportedOperationException("GraphBinaryIo is only used to support IoRegistry configuration - it's methods are not implemented"); - } - - @Override - public Mapper.Builder mapper() { - throw new UnsupportedOperationException("GraphBinaryIo is only used to support IoRegistry configuration - it's methods are not implemented"); - } - - @Override - public void writeGraph(final String file) throws IOException { - throw new UnsupportedOperationException("GraphBinaryIo is only used to support IoRegistry configuration - it's methods are not implemented"); - } - - @Override - public void readGraph(final String file) throws IOException { - throw new UnsupportedOperationException("GraphBinaryIo is only used to support IoRegistry configuration - it's methods are not implemented"); - } -} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java index e181393de7..24988ed084 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/GremlinLangTest.java @@ -28,7 +28,10 @@ import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex; import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; +import org.apache.tinkerpop.gremlin.structure.io.pdt.ProviderDefined; import org.apache.tinkerpop.gremlin.structure.io.pdt.ProviderDefinedType; +import org.apache.tinkerpop.gremlin.structure.io.pdt.ProviderDefinedTypeAdapter; +import org.apache.tinkerpop.gremlin.structure.io.pdt.ProviderDefinedTypeRegistry; import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceEdge; import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceVertex; import org.apache.tinkerpop.gremlin.util.DatetimeHelper; @@ -450,6 +453,45 @@ public class GremlinLangTest { } } + public static class AdapterPrecedenceTests { + + /** + * A type annotated with @ProviderDefined that also has an explicit adapter registered. + * The adapter should take precedence over the annotation. + */ + @ProviderDefined(name = "AnnotationName") + private static class DualType { + public int value = 42; + + private DualType() {} + DualType(final int value) { this.value = value; } + } + + @Test + public void shouldUseAdapterOverAnnotation() { + final ProviderDefinedTypeRegistry registry = ProviderDefinedTypeRegistry.empty(); + registry.register(new ProviderDefinedTypeAdapter<DualType>() { + @Override public String typeName() { return "AdapterName"; } + @Override public Class<DualType> targetClass() { return DualType.class; } + @Override public Map<String, Object> toProperties(final DualType obj) { + return Collections.singletonMap("v", obj.value); + } + @Override public DualType fromProperties(final Map<String, Object> properties) { + return new DualType((int) properties.get("v")); + } + }); + + final GraphTraversalSource g2 = traversal().with(EmptyGraph.instance()); + g2.getGremlinLang().setPdtRegistry(registry); + final String gremlin = g2.inject(new DualType(7)).asAdmin().getGremlinLang().getGremlin(); + + // adapter produces "AdapterName" with key "v", not annotation's "AnnotationName" with key "value" + assertTrue(gremlin, gremlin.contains("PDT(\"AdapterName\"")); + assertTrue(gremlin, gremlin.contains("\"v\":7")); + assertFalse(gremlin, gremlin.contains("AnnotationName")); + } + } + public static class UnsupportedTypeTests { /** diff --git a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs index fff3dcff88..bb48d96bc6 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GremlinLang.cs @@ -375,7 +375,8 @@ namespace Gremlin.Net.Process.Traversal if (arg is Type type) return type.Name; - // Registry-based dehydration + // Precedence: a registered adapter intentionally takes priority over the [ProviderDefined] + // attribute so that explicit adapters can override attribute-derived dehydration behavior. if (PdtRegistry != null) { var adapterInfo = PdtRegistry.GetAdapterByType(arg.GetType()); diff --git a/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs b/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs index df80b589a6..a0c15d1ef5 100644 --- a/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.UnitTest/Process/Traversal/GremlinLangTests.cs @@ -1174,5 +1174,40 @@ namespace Gremlin.Net.UnitTest.Process.Traversal public int X { get; set; } public int Y { get; set; } } + + [Fact] + public void g_Inject_PDT_adapter_takes_precedence_over_attribute() + { + var registry = new ProviderDefinedTypeRegistry(); + registry.Register(new AdapterForAnnotatedPoint()); + + var g = new GraphTraversalSource(); + g.GremlinLang.PdtRegistry = registry; + + var point = new AnnotatedPointWithAdapter { X = 5, Y = 10 }; + var result = g.Inject((object)point).GremlinLang.GetGremlin(); + + // The adapter uses type name "adapter.Point" and only exposes "a"/"b" properties, + // overriding the attribute which would produce "attr.Point" with "X"/"Y". + Assert.Equal("g.inject(PDT(\"adapter.Point\",[\"a\":5,\"b\":10]))", result); + } + + [ProviderDefined(Name = "attr.Point")] + private class AnnotatedPointWithAdapter + { + public int X { get; set; } + public int Y { get; set; } + } + + private class AdapterForAnnotatedPoint : IProviderDefinedTypeAdapter<AnnotatedPointWithAdapter> + { + public string TypeName => "adapter.Point"; + + public AnnotatedPointWithAdapter FromProperties(IReadOnlyDictionary<string, object?> properties) => + new() { X = (int)properties["a"]!, Y = (int)properties["b"]! }; + + public IReadOnlyDictionary<string, object?> ToProperties(AnnotatedPointWithAdapter obj) => + new Dictionary<string, object?> { { "a", obj.X }, { "b", obj.Y } }; + } } } diff --git a/gremlin-go/driver/gremlinlang.go b/gremlin-go/driver/gremlinlang.go index 59b900eab4..dac9e64aa8 100644 --- a/gremlin-go/driver/gremlinlang.go +++ b/gremlin-go/driver/gremlinlang.go @@ -299,7 +299,9 @@ func (gl *GremlinLang) argAsString(arg interface{}) (string, error) { case []byte: return fmt.Sprintf("Binary(\"%s\")", base64.StdEncoding.EncodeToString(v)), nil default: - // Registry-based dehydration + // Registry-based dehydration: a registered adapter intentionally takes precedence + // over any reflection/struct-based fallback, allowing explicit adapters to override + // default behavior for a given Go type. if gl.pdtRegistry != nil { adapter := gl.pdtRegistry.GetAdapterByType(reflect.TypeOf(arg)) if adapter != nil && adapter.ToProperties != nil { diff --git a/gremlin-go/driver/gremlinlang_test.go b/gremlin-go/driver/gremlinlang_test.go index c857fb5a91..9ab9b218f0 100644 --- a/gremlin-go/driver/gremlinlang_test.go +++ b/gremlin-go/driver/gremlinlang_test.go @@ -22,6 +22,7 @@ package gremlingo import ( "fmt" "math" + "reflect" "regexp" "strings" "testing" @@ -921,3 +922,29 @@ func Test_PDT_GremlinLang(t *testing.T) { } }) } + +// adapterPrecedencePoint is a struct type used to verify that a registered adapter +// takes precedence over the default handling (which would panic for an unknown struct). +type adapterPrecedencePoint struct { + X int32 + Y int32 +} + +func Test_PDT_AdapterPrecedenceOverDefault(t *testing.T) { + registry := NewPDTRegistry() + registry.RegisterFuncsWithType("test:Point", reflect.TypeOf(adapterPrecedencePoint{}), + nil, + func(obj interface{}) (map[string]interface{}, error) { + p := obj.(adapterPrecedencePoint) + return map[string]interface{}{"x": p.X, "y": p.Y}, nil + }) + + g := NewGraphTraversalSource(nil, nil) + g.GetGremlinLang().pdtRegistry = registry + + gremlin := g.Inject(adapterPrecedencePoint{X: 7, Y: 9}).GremlinLang.GetGremlin() + expected := `g.inject(PDT("test:Point",["x":7,"y":9]))` + if gremlin != expected { + t.Errorf("adapter precedence: got %v, expected %v", gremlin, expected) + } +} diff --git a/gremlin-python/src/main/python/gremlin_python/process/traversal.py b/gremlin-python/src/main/python/gremlin_python/process/traversal.py index 93f835c57f..6b8bcbfea3 100644 --- a/gremlin-python/src/main/python/gremlin_python/process/traversal.py +++ b/gremlin-python/src/main/python/gremlin_python/process/traversal.py @@ -952,7 +952,9 @@ class GremlinLang(object): if isinstance(arg, type): return arg.__name__ - # Registry-based dehydration + # Registry-based dehydration — a registered adapter intentionally takes + # precedence over the @provider_defined decorator fallback below, allowing + # explicit adapters to override decorator-derived behavior. if self.pdt_registry is not None: adapter = self.pdt_registry.get_adapter_by_class(type(arg)) if adapter is not None and adapter['serialize'] is not None: diff --git a/gremlin-python/src/main/python/gremlin_python/structure/graph.py b/gremlin-python/src/main/python/gremlin_python/structure/graph.py index 61cec674d8..6160af3c10 100644 --- a/gremlin-python/src/main/python/gremlin_python/structure/graph.py +++ b/gremlin-python/src/main/python/gremlin_python/structure/graph.py @@ -149,6 +149,8 @@ class ProviderDefinedType(object): raise ValueError("name cannot be null or empty") self._name = name self._properties = dict(properties) if properties else {} + if any(not isinstance(k, str) for k in self._properties): + raise TypeError("ProviderDefinedType property keys must be strings") @property def name(self): diff --git a/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py b/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py index 74a386163d..b73f7b8ee2 100644 --- a/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py +++ b/gremlin-python/src/main/python/tests/unit/process/test_gremlin_lang.py @@ -594,6 +594,28 @@ class TestGremlinLang(object): gremlin = g.inject(p).gremlin_lang.get_gremlin() assert "PDT('com.example.Point',['x':1,'y':2])" in gremlin + def test_pdt_adapter_takes_precedence_over_decorator(self): + from gremlin_python.structure.graph import ProviderDefinedTypeRegistry, provider_defined + g = traversal().with_(None) + + @provider_defined(name="com.example.Point") + class Point: + def __init__(self, x, y): + self.x = x + self.y = y + + registry = ProviderDefinedTypeRegistry() + registry.register("com.adapter.Point", + deserialize_fn=lambda props: Point(props["a"], props["b"]), + serialize_fn=lambda p: {"a": p.x, "b": p.y}, + target_class=Point) + g.gremlin_lang.pdt_registry = registry + + p = Point(3, 4) + gremlin = g.inject(p).gremlin_lang.get_gremlin() + # The adapter's type_name and properties must win over the decorator's + assert "PDT('com.adapter.Point',['a':3,'b':4])" in gremlin + def test_pdt_special_characters_in_name(self): from gremlin_python.structure.graph import ProviderDefinedType g = traversal().with_(None) diff --git a/gremlin-python/src/main/python/tests/unit/structure/io/test_provider_defined_type.py b/gremlin-python/src/main/python/tests/unit/structure/io/test_provider_defined_type.py index 2d5a47cb08..32deaa9840 100644 --- a/gremlin-python/src/main/python/tests/unit/structure/io/test_provider_defined_type.py +++ b/gremlin-python/src/main/python/tests/unit/structure/io/test_provider_defined_type.py @@ -35,6 +35,10 @@ class TestProviderDefinedType(object): with pytest.raises(ValueError): ProviderDefinedType(None, {"x": 1}) + def test_non_string_key_rejected(self): + with pytest.raises(TypeError): + ProviderDefinedType("com.example.Bad", {1: "value"}) + class TestProviderDefinedTypeRegistry(object): diff --git a/gremlin-server/pom.xml b/gremlin-server/pom.xml index 9b40be61ee..df4c5e6da6 100644 --- a/gremlin-server/pom.xml +++ b/gremlin-server/pom.xml @@ -173,16 +173,6 @@ limitations under the License. </archive> </configuration> </plugin> - <plugin> - <artifactId>maven-jar-plugin</artifactId> - <executions> - <execution> - <goals> - <goal>test-jar</goal> - </goals> - </execution> - </executions> - </plugin> <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-surefire-plugin</artifactId>
