Tatenda-k commented on code in PR #3861:
URL: https://github.com/apache/fory/pull/3861#discussion_r3616257251
##########
java/fory-core/src/main/java/org/apache/fory/builder/CompatibleCodecBuilder.java:
##########
@@ -386,22 +463,93 @@ private Expression skipField(Descriptor descriptor) {
remoteFieldInfo(descriptor));
}
+ @Override
+ protected Expression getFieldValue(Expression bean, Descriptor descriptor) {
+ if (extraFieldsSinkField != null && descriptor.getField() == null) {
+ if (extraFieldsSinkExpr == null || extraFieldsSinkExprBean != bean) {
+ extraFieldsSinkExprBean = bean;
+ extraFieldsSinkExpr =
+ tryInlineCast(
+ new Invoke(extraFieldsSinkAccessorExpr(), "getObject",
OBJECT_TYPE, bean),
+ TypeRef.of(ForyExtraFields.class));
+ }
+ Invoke raw =
+ new Invoke(
+ extraFieldsSinkExpr, "get", OBJECT_TYPE,
Literal.ofString(descriptor.getName()));
+ return tryInlineCast(raw, descriptor.getTypeRef());
+ }
+ return super.getFieldValue(bean, descriptor);
+ }
+
@Override
protected Expression setFieldValue(Expression bean, Descriptor descriptor,
Expression value) {
if (descriptor.getField() == null) {
FieldConverter<?> converter = descriptor.getFieldConverter();
if (converter != null) {
return setFieldConverterTargetValue(bean, descriptor, converter,
value);
}
- // Field doesn't exist in current class, skip set this field value.
+ // If Field doesn't exist in current class,
+ if (extraFieldsSinkField != null) {
+ return new StaticInvoke(
+ ForyExtraFields.class,
+ "capture",
+ TypeRef.of(void.class),
+ bean,
+ extraFieldsSinkAccessorExpr(),
+ typeDefFieldExpr(),
+ Literal.ofString(descriptor.getName()),
+ value);
+ }
// Note that the field value shouldn't be an inlined value, otherwise
field value read may
- // be ignored.
- // Add an ignored call here to make expression type to void.
+ // be ignored. Add an ignored call here to make expression type to void.
return new StaticInvoke(ExceptionUtils.class, "ignore", value);
}
return super.setFieldValue(bean, descriptor, value);
}
+ private String ensureLazyFieldGenerated(
+ String cachedFieldName, String nameConstant, TypeRef<?> fieldType,
Expression initializer) {
+ if (cachedFieldName == null) {
+ cachedFieldName = ctx.newName(nameConstant);
+ ctx.addField(false, true, ctx.type(fieldType), cachedFieldName,
initializer);
+ }
+ return cachedFieldName;
+ }
+
+ private Expression extraFieldsSinkAccessorExpr() {
+ extraFieldsSinkAccessorName =
+ ensureLazyFieldGenerated(
+ extraFieldsSinkAccessorName,
+ EXTRA_FIELDS_SINK_ACCESSOR_NAME,
+ TypeRef.of(FieldAccessor.class),
+ new StaticInvoke(
+ FieldAccessor.class,
+ "createAccessor",
+ TypeRef.of(FieldAccessor.class),
+ getOrCreateField(
Review Comment:
The accessor now resolves through the sink field's own declaring class
`extraFieldsSinkField.getDeclaringClass()`, and added
`hiddenInheritedSinkCapturesIntoDeclaringClassField` which reproduces the above
scenario.
##########
java/fory-core/src/main/java/org/apache/fory/serializer/ForyExtraFields.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.fory.serializer;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.fory.collection.ClassValueCache;
+import org.apache.fory.meta.TypeDef;
+import org.apache.fory.reflect.FieldAccessor;
+
+/**
+ * Opt-in sink for compatible-mode extra fields. A class participates by
declaring a field of this
+ * type; the serialization framework detects it, excludes it from the normal
field set, and routes
+ * unmatched remote fields into it instead of discarding them.
+ *
+ * <p>The remote {@link TypeDef} is stored transiently so that on
re-serialization the framework can
+ * emit the remote schema header and replay all fields in their original
order, allowing a
+ * downstream peer with the full schema to recover them.
+ */
+public final class ForyExtraFields {
+
+ private final Map<String, Object> fields = new HashMap<>();
+
+ public Object get(String name) {
+ return fields.get(name);
+ }
+
+ public Object getOrDefault(String name, Object defaultValue) {
+ return fields.getOrDefault(name, defaultValue);
+ }
+
+ public boolean isEmpty() {
+ return fields.isEmpty();
+ }
+
+ public boolean containsKey(String name) {
+ return fields.containsKey(name);
+ }
+
+ Object put(String name, Object value) {
+ return fields.put(name, value);
+ }
+
+ /**
+ * GC-transparent class-keyed cache: on JVM this is backed by ClassValue so
entries do not prevent
+ * the associated Class (or its ClassLoader) from being collected. On
Android/GraalVM it falls
+ * back to a ConcurrentHashMap.
+ */
+ private static final ClassValueCache<Optional<FieldAccessor>> SINK_CACHE =
+ ClassValueCache.newClassKeyCache(16);
+
+ public static Field findSinkField(Class<?> cls) {
+ return SINK_CACHE
+ .get(cls, () -> scanForExtraField(cls))
+ .map(FieldAccessor::getField)
+ .orElse(null);
+ }
+
+ /** Returns a {@link FieldAccessor} for the {@link ForyExtraFields} sink
field on {@code cls}. */
+ public static FieldAccessor findSinkAccessor(Class<?> cls) {
+ return SINK_CACHE.get(cls, () -> scanForExtraField(cls)).orElse(null);
+ }
+
+ private static Optional<FieldAccessor> scanForExtraField(Class<?> cls) {
+ for (Class<?> c = cls; c != null && c != Object.class; c =
c.getSuperclass()) {
+ for (Field f : c.getDeclaredFields()) {
+ if (ForyExtraFields.class == f.getType()) {
Review Comment:
Updated the above method to check for and ignore static fields, and log a
warning if a static field is encountered. Also and added the
staticSinkFieldIgnoredByDiscovery` test to ensure this.
##########
java/fory-core/src/main/java/org/apache/fory/serializer/ForyExtraFields.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.fory.serializer;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.fory.collection.ClassValueCache;
+import org.apache.fory.meta.TypeDef;
+import org.apache.fory.reflect.FieldAccessor;
+
+/**
+ * Opt-in sink for compatible-mode extra fields. A class participates by
declaring a field of this
+ * type; the serialization framework detects it, excludes it from the normal
field set, and routes
Review Comment:
Updated `buildFieldDescriptors()` to exclude a `ForyExtraField` instance and
added `sinkFieldAbsentFromLocalTypeDef()` and
`inheritedSinkFieldAbsentFromLocalTypeDef()` tests to confirm the exclusion.
##########
java/fory-core/src/main/java/org/apache/fory/resolver/TypeResolver.java:
##########
@@ -1305,9 +1334,17 @@ private TypeInfo getMetaSharedTypeInfo(TypeDef typeDef,
Class<?> clz) {
if (StaticGeneratedStructSerializer.class.isAssignableFrom(sc)) {
typeInfo.setSerializer(this, newStaticGeneratedStructSerializer(sc, cls,
typeDef));
} else if (sc == CompatibleSerializer.class) {
- typeInfo.setSerializer(this, new CompatibleSerializer(this, cls,
typeDef));
+ CompatibleSerializer<?> cs = new CompatibleSerializer<>(this, cls,
typeDef);
Review Comment:
Noted. I will update the pr description.
##########
java/fory-core/src/main/java/org/apache/fory/resolver/TypeInfo.java:
##########
@@ -170,6 +174,15 @@ public void setSerializer(Serializer<?> serializer) {
void setSerializer(TypeResolver resolver, Serializer<?> serializer) {
this.serializer = serializer;
needToWriteTypeDef = serializer != null &&
resolver.needToWriteTypeDef(serializer);
+ this.extraFieldsSinkAccessor = type == null ? null :
ForyExtraFields.findSinkAccessor(type);
Review Comment:
Fixed both `copy(...)` overloads to preserve the `extraFieldsSinkAccessor`
on the copy. Added `copyWith`sinkAccessorSurvivesNumericIdRegistration` test
to enact the sequence you flagged. I didn't initialize the
`extraFieldsSinkAccessor` in the first and second constructor because the
`typeResolver` is not available there.
##########
java/fory-core/src/main/java/org/apache/fory/resolver/TypeResolver.java:
##########
@@ -514,6 +515,30 @@ public boolean isMapDescriptor(Descriptor descriptor) {
public abstract TypeInfo getTypeInfo(Class<?> cls, TypeInfoHolder
classInfoHolder);
+ public final TypeInfo getTypeInfoByTypeDefId(long typeDefId) {
+ return extRegistry.typeInfoByTypeDefId.get(typeDefId);
+ }
+
+ public final Serializer<?> getExtraFieldsWriteSerializer(Class<?> cls, long
typeDefId) {
+ Map<Long, Serializer<?>> idToSerializer =
extRegistry.extraFieldsSerializers.get(cls);
+ return idToSerializer != null ? idToSerializer.get(typeDefId) : null;
+ }
+
+ private static boolean hasExtraFieldsSinkField(Class<?> cls) {
+ return ForyExtraFields.findSinkField(cls) != null;
+ }
+
+ /**
+ * Records the reader-class serializer used to replay {@code cls} under the
remote {@code
+ * typeDefId} it was captured from.
+ */
+ private void registerExtraFieldsSerializer(Class<?> cls, long typeDefId,
Serializer<?> gen) {
+ extRegistry
+ .extraFieldsSerializers
+ .computeIfAbsent(cls, k -> new ConcurrentHashMap<>())
+ .putIfAbsent(typeDefId, gen);
Review Comment:
Fixed the handoff race: `put()` is used for the generated serializer,
`putIfAbsent()` for the interpreter. Updated `roundTripAfterJitCompilation` to
assert on the replay cache owner -`getExtraFieldsWriteSerializer` instead of
the `TypeInfo` serializer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
