feat: add Apache Fory serialization support for SOFARPC#1551
Open
sunhailin-Leo wants to merge 3 commits intosofastack:masterfrom
Open
feat: add Apache Fory serialization support for SOFARPC#1551sunhailin-Leo wants to merge 3 commits intosofastack:masterfrom
sunhailin-Leo wants to merge 3 commits intosofastack:masterfrom
Conversation
- Add new codec-sofa-fory module with Apache Fory (org.apache.fory:fory-core:0.16.0) - Implement ForySerializer with DISABLE/WARN/STRICT security check modes - Implement SofaRequestForySerializer and SofaResponseForySerializer - Register as Extension(value="fory", code=23) to avoid conflict with legacy fury2(code=22) - Add unit tests covering encode/decode, SofaRequest/SofaResponse, and checker modes - Add fory.version=0.16.0 to bom/pom.xml Closes sofastack#1544
Contributor
📝 WalkthroughWalkthroughAdds a new codec module implementing Apache Fory serialization for SOFARPC: dependency and module entries, a thread-local Fory-based Serializer extension, request/response custom serializers, SPI registration, test models/resources, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ForySerializer
participant ThreadSafeFory as Fory
participant CustomSerializer
Client->>ForySerializer: encode(object, context)
ForySerializer->>Fory: set classloader / obtain thread-local instance
alt has CustomSerializer
ForySerializer->>CustomSerializer: encodeObject(object, context)
CustomSerializer-->>ForySerializer: AbstractByteBuf
else use core Fory
ForySerializer->>Fory: serialize(object into MemoryBuffer)
Fory-->>ForySerializer: bytes
ForySerializer->>ForySerializer: wrap bytes -> ByteBuf
end
ForySerializer-->>Client: AbstractByteBuf
Client->>ForySerializer: decode(data, Class/Template, context)
ForySerializer->>Fory: set classloader / obtain thread-local instance
alt has CustomSerializer
ForySerializer->>CustomSerializer: decodeObject(...)
CustomSerializer-->>ForySerializer: object/template
else use core Fory
ForySerializer->>Fory: deserialize(MemoryBuffer)
Fory-->>ForySerializer: object
end
ForySerializer-->>Client: object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java (3)
153-154: Same exception re-wrapping issue in bothdecodemethods.Apply the same pattern to preserve original
SofaRpcExceptioninstances.Also applies to: 175-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java` around lines 153 - 154, In ForySerializer's decode methods, avoid re-wrapping existing SofaRpcException instances: inside the catch(Exception e) blocks in decode (and the similar block at lines ~175-176), check if e is an instance of SofaRpcException and rethrow it directly (throw (SofaRpcException) e); otherwise throw buildDeserializeError(e.getMessage(), e). This preserves original exception types and stack traces while still converting other exceptions to the deserialize error via buildDeserializeError.
140-142: Error message doesn't fully describe the validation.The condition checks both
data.readableBytes() <= 0andclazz == null, but the error message only mentions "empty array".♻️ Proposed fix for clearer error message
public Object decode(final AbstractByteBuf data, final Class clazz, final Map<String, String> context) throws SofaRpcException { - if (data.readableBytes() <= 0 || clazz == null) { - throw buildDeserializeError("Deserialized array is empty."); + if (clazz == null) { + throw buildDeserializeError("Target class is null."); + } + if (data.readableBytes() <= 0) { + throw buildDeserializeError("Deserialized array is empty."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java` around lines 140 - 142, The validation in ForySerializer's method checks both data.readableBytes() <= 0 and clazz == null but the thrown error created via buildDeserializeError only mentions an "empty array"; update the validation to produce a clearer message by including both failure causes (e.g., "Deserialized array is empty or target class is null") or perform two separate checks and throw specific errors for data.readableBytes() <= 0 and for clazz == null so the exception indicates which condition failed; target symbols: ForySerializer, data, clazz, and buildDeserializeError.
130-131: Exception re-wrapping may obscure custom serializer errors.If
customSerializer.encodeObject()throwsSofaRpcException, the catch block wraps it again viabuildSerializeError(), potentially losing the original error type.♻️ Proposed fix to preserve original SofaRpcException
+ } catch (SofaRpcException e) { + throw e; } catch (Exception e) { throw buildSerializeError(e.getMessage(), e); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java` around lines 130 - 131, The catch block in ForySerializer around customSerializer.encodeObject currently wraps all Exceptions with buildSerializeError, which hides original SofaRpcException; update the catch to detect if the caught exception is an instance of SofaRpcException and rethrow it unchanged, otherwise continue to wrap other exceptions with buildSerializeError (use instanceof check on the caught Exception in the method where customSerializer.encodeObject is called and conditionally rethrow or wrap).codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java (2)
276-284: Same test isolation concern for DISABLE mode test.Apply the same try-finally pattern here for consistent cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java` around lines 276 - 284, The DISABLE mode test sets the system property "sofa.rpc.codec.serialize.checkMode" but does not guarantee cleanup on failure; wrap the property set, ForySerializer usage (ForySerializer disableForySerializer and its encode(...) calls) in a try-finally so System.getProperties().remove("sofa.rpc.codec.serialize.checkMode") always runs, mirroring the pattern used for other modes to ensure test isolation and consistent cleanup.
247-274: Consider using try-finally for system property cleanup to ensure test isolation.If an assertion fails between setting and removing the system property (e.g., lines 251-273), the property may leak and affect subsequent tests.
♻️ Proposed refactor for safer property cleanup
// test WARN mode + try { - System.getProperties().put("sofa.rpc.codec.serialize.checkMode", "WARN"); + System.getProperties().put("sofa.rpc.codec.serialize.checkMode", "WARN"); - ForySerializer warnForySerializer = new ForySerializer(); + ForySerializer warnForySerializer = new ForySerializer(); - warnForySerializer.encode(noneClassNullBlackClass, null); + warnForySerializer.encode(noneClassNullBlackClass, null); - try { - warnForySerializer.encode(noneClassHasBlackClass, null); - Assert.fail(); - } catch (Exception e) { - // expected - } + try { + warnForySerializer.encode(noneClassHasBlackClass, null); + Assert.fail(); + } catch (Exception e) { + // expected + } // ... remaining assertions ... - System.getProperties().remove("sofa.rpc.codec.serialize.checkMode"); + } finally { + System.getProperties().remove("sofa.rpc.codec.serialize.checkMode"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java` around lines 247 - 274, Wrap the block that sets and removes the system property "sofa.rpc.codec.serialize.checkMode" in a try-finally so the property is always removed even if assertions fail; specifically, around the creation and use of ForySerializer and its encode(...) calls (the warnForySerializer variable and its calls to encode with noneClassNullBlackClass, noneClassHasBlackClass, blackListClass, whiteClassNullBlackClass, whiteClassHasBlackClass), move System.getProperties().put(...) before a try and put System.getProperties().remove("sofa.rpc.codec.serialize.checkMode") in the finally to guarantee cleanup and test isolation.codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java (3)
68-81: Same exception re-wrapping issue indecodeObject.The
SofaRpcExceptionthrown at line 72 will be caught and re-wrapped at line 80.♻️ Proposed fix
sofaRequest.setMethodArgs(args); return sofaRequest; + } catch (SofaRpcException e) { + throw e; } catch (Exception e) { throw new SofaRpcException(RpcErrorType.SERVER_DESERIALIZE, e.getMessage(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java` around lines 68 - 81, The decode path in SofaRequestForySerializer currently throws a SofaRpcException inside the try which is then always caught and re-wrapped; update the exception handling in the block that surrounds the deserialization (the try/catch around fory.deserialize calls in SofaRequestForySerializer.decodeObject) so that if the caught Exception is already a SofaRpcException you rethrow it unchanged, otherwise wrap non-SofaRpcException errors in a new SofaRpcException with context; specifically change the catch to check "if (e instanceof SofaRpcException) throw (SofaRpcException) e;" before creating a new SofaRpcException to avoid double-wrapping.
49-62: Exception re-wrapping loses original error type.The
SofaRpcExceptionthrown at line 53 for generic calls will be caught and re-wrapped at lines 60-61, potentially losing context.♻️ Proposed fix to preserve original SofaRpcException
fory.serialize(writeBuffer, object); final Object[] args = object.getMethodArgs(); fory.serialize(writeBuffer, args); return new ByteArrayWrapperByteBuf(writeBuffer.getBytes(0, writeBuffer.writerIndex())); + } catch (SofaRpcException e) { + throw e; } catch (Exception e) { throw new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, e.getMessage(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java` around lines 49 - 62, The catch block in SofaRequestForySerializer currently re-wraps every Exception (including SofaRpcException thrown for generic calls) into a new SofaRpcException, losing the original error type; update the catch logic in the serialize method to detect if the caught exception is already a SofaRpcException and rethrow it as-is (or preserve it as the cause) instead of creating a new wrapper—i.e., in the catch (Exception e) block, if (e instanceof SofaRpcException) throw (SofaRpcException) e; otherwise throw new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, e.getMessage(), e); so that exceptions raised by the generic call check (the explicit throw new SofaRpcException(...)) retain their original type and context.
90-108: Same exception re-wrapping issue indecodeObjectByTemplate.The
SofaRpcExceptionthrown at line 95 will be caught and re-wrapped at line 107.♻️ Proposed fix
template.setMethodArgs(args); + } catch (SofaRpcException e) { + throw e; } catch (Exception e) { throw new SofaRpcException(RpcErrorType.SERVER_DESERIALIZE, e.getMessage(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java` around lines 90 - 108, In decodeObjectByTemplate in SofaRequestForySerializer, stop re-wrapping an existing SofaRpcException: change the catch to detect if the caught Exception is already a SofaRpcException and rethrow it (throw (SofaRpcException) e), otherwise wrap non-SofaRpcException into a new SofaRpcException as currently done; this ensures exceptions thrown inside the try (e.g., the explicit throw new SofaRpcException(...)) are propagated without double-wrapping.codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java (2)
57-67: Exception re-wrapping loses original error type.When the generic-call check throws
SofaRpcExceptionat line 62, the outer catch at line 65 re-wraps it, convertingCLIENT_SERIALIZEto anotherCLIENT_SERIALIZEbut losing the specific message context.♻️ Proposed fix to preserve original SofaRpcException
try { boolean genericSerialize = context != null && isGenericResponse( context.get(RemotingConstants.HEAD_GENERIC_TYPE)); if (genericSerialize) { // TODO support generic call throw new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, "Generic call is not supported for now."); } return (SofaResponse) fory.deserialize(readBuffer); + } catch (SofaRpcException e) { + throw e; } catch (Exception e) { throw new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, e.getMessage(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java` around lines 57 - 67, The catch in SofaResponseForySerializer currently wraps every Exception (including the intentionally thrown SofaRpcException from the generic-call check) into a new SofaRpcException, losing the original exception type and message; update the catch in the method that calls isGenericResponse/for y.deserialize so that if the caught exception is already a SofaRpcException it is rethrown unchanged (throw e) and only non-SofaRpcException instances are wrapped into a new SofaRpcException with the original as the cause—this preserves the original error type and message while still handling unexpected exceptions.
76-91: Same exception re-wrapping issue indecodeObjectByTemplate.The
SofaRpcExceptionthrown at line 82 for generic calls will be caught and re-wrapped at line 89-90.♻️ Proposed fix
} catch (SofaRpcException e) { + throw e; + } catch (Exception e) { throw new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, e.getMessage(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java` around lines 76 - 91, The decodeObjectByTemplate logic throws a new SofaRpcException for generic calls which is then immediately caught and re-wrapped; modify the catch in SofaResponseForySerializer.decodeObjectByTemplate so it rethrows the original SofaRpcException instead of wrapping it again: in the catch block, if (e instanceof SofaRpcException) throw (SofaRpcException) e; otherwise wrap non-SofaRpcException exceptions in a new SofaRpcException as currently done. This preserves the original error type/message from the isGenericResponse branch that throws new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bom/pom.xml`:
- Line 43: Update the declared Apache Fory version from 0.16.0 to 0.15.0 by
changing the <fory.version> property and ensure the dependency management entry
that references org.apache.fory:fory-core (the block that uses the fory.version
property) is consistent with the updated property; search for the fory.version
property and the dependencyManagement entry that references fory-core to update
them both to 0.15.0.
In
`@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java`:
- Around line 73-76: Replace the cast to ClassResolver from getTypeResolver()
with a direct call to getClassResolver() and update the comment to reference
setClassChecker(ClassChecker) instead of setTypeChecker(TypeChecker);
specifically change the code that now does ClassResolver classResolver =
(ClassResolver) foryInstance.getTypeResolver() to use
foryInstance.getClassResolver() and correct the comment text in ForySerializer
(and any related methods that mention setTypeChecker) so it refers to
setClassChecker(ClassChecker) and obtaining the ClassResolver via
getClassResolver().
In
`@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java`:
- Around line 49-51: The catch in SofaResponseForySerializer currently throws a
SofaRpcException with RpcErrorType.SERVER_DESERIALIZE for a serialization
failure; update the error type to the correct serialization constant (use
RpcErrorType.SERVER_SERIALIZE since this is encoding a response on the server
side) in the catch block that wraps exceptions during serializeResponse (or the
serializer logic) so the exception accurately reflects a serialization error.
---
Nitpick comments:
In
`@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java`:
- Around line 153-154: In ForySerializer's decode methods, avoid re-wrapping
existing SofaRpcException instances: inside the catch(Exception e) blocks in
decode (and the similar block at lines ~175-176), check if e is an instance of
SofaRpcException and rethrow it directly (throw (SofaRpcException) e); otherwise
throw buildDeserializeError(e.getMessage(), e). This preserves original
exception types and stack traces while still converting other exceptions to the
deserialize error via buildDeserializeError.
- Around line 140-142: The validation in ForySerializer's method checks both
data.readableBytes() <= 0 and clazz == null but the thrown error created via
buildDeserializeError only mentions an "empty array"; update the validation to
produce a clearer message by including both failure causes (e.g., "Deserialized
array is empty or target class is null") or perform two separate checks and
throw specific errors for data.readableBytes() <= 0 and for clazz == null so the
exception indicates which condition failed; target symbols: ForySerializer,
data, clazz, and buildDeserializeError.
- Around line 130-131: The catch block in ForySerializer around
customSerializer.encodeObject currently wraps all Exceptions with
buildSerializeError, which hides original SofaRpcException; update the catch to
detect if the caught exception is an instance of SofaRpcException and rethrow it
unchanged, otherwise continue to wrap other exceptions with buildSerializeError
(use instanceof check on the caught Exception in the method where
customSerializer.encodeObject is called and conditionally rethrow or wrap).
In
`@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java`:
- Around line 68-81: The decode path in SofaRequestForySerializer currently
throws a SofaRpcException inside the try which is then always caught and
re-wrapped; update the exception handling in the block that surrounds the
deserialization (the try/catch around fory.deserialize calls in
SofaRequestForySerializer.decodeObject) so that if the caught Exception is
already a SofaRpcException you rethrow it unchanged, otherwise wrap
non-SofaRpcException errors in a new SofaRpcException with context; specifically
change the catch to check "if (e instanceof SofaRpcException) throw
(SofaRpcException) e;" before creating a new SofaRpcException to avoid
double-wrapping.
- Around line 49-62: The catch block in SofaRequestForySerializer currently
re-wraps every Exception (including SofaRpcException thrown for generic calls)
into a new SofaRpcException, losing the original error type; update the catch
logic in the serialize method to detect if the caught exception is already a
SofaRpcException and rethrow it as-is (or preserve it as the cause) instead of
creating a new wrapper—i.e., in the catch (Exception e) block, if (e instanceof
SofaRpcException) throw (SofaRpcException) e; otherwise throw new
SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, e.getMessage(), e); so that
exceptions raised by the generic call check (the explicit throw new
SofaRpcException(...)) retain their original type and context.
- Around line 90-108: In decodeObjectByTemplate in SofaRequestForySerializer,
stop re-wrapping an existing SofaRpcException: change the catch to detect if the
caught Exception is already a SofaRpcException and rethrow it (throw
(SofaRpcException) e), otherwise wrap non-SofaRpcException into a new
SofaRpcException as currently done; this ensures exceptions thrown inside the
try (e.g., the explicit throw new SofaRpcException(...)) are propagated without
double-wrapping.
In
`@codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java`:
- Around line 57-67: The catch in SofaResponseForySerializer currently wraps
every Exception (including the intentionally thrown SofaRpcException from the
generic-call check) into a new SofaRpcException, losing the original exception
type and message; update the catch in the method that calls
isGenericResponse/for y.deserialize so that if the caught exception is already a
SofaRpcException it is rethrown unchanged (throw e) and only
non-SofaRpcException instances are wrapped into a new SofaRpcException with the
original as the cause—this preserves the original error type and message while
still handling unexpected exceptions.
- Around line 76-91: The decodeObjectByTemplate logic throws a new
SofaRpcException for generic calls which is then immediately caught and
re-wrapped; modify the catch in
SofaResponseForySerializer.decodeObjectByTemplate so it rethrows the original
SofaRpcException instead of wrapping it again: in the catch block, if (e
instanceof SofaRpcException) throw (SofaRpcException) e; otherwise wrap
non-SofaRpcException exceptions in a new SofaRpcException as currently done.
This preserves the original error type/message from the isGenericResponse branch
that throws new SofaRpcException(RpcErrorType.CLIENT_SERIALIZE, ...).
In
`@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java`:
- Around line 276-284: The DISABLE mode test sets the system property
"sofa.rpc.codec.serialize.checkMode" but does not guarantee cleanup on failure;
wrap the property set, ForySerializer usage (ForySerializer
disableForySerializer and its encode(...) calls) in a try-finally so
System.getProperties().remove("sofa.rpc.codec.serialize.checkMode") always runs,
mirroring the pattern used for other modes to ensure test isolation and
consistent cleanup.
- Around line 247-274: Wrap the block that sets and removes the system property
"sofa.rpc.codec.serialize.checkMode" in a try-finally so the property is always
removed even if assertions fail; specifically, around the creation and use of
ForySerializer and its encode(...) calls (the warnForySerializer variable and
its calls to encode with noneClassNullBlackClass, noneClassHasBlackClass,
blackListClass, whiteClassNullBlackClass, whiteClassHasBlackClass), move
System.getProperties().put(...) before a try and put
System.getProperties().remove("sofa.rpc.codec.serialize.checkMode") in the
finally to guarantee cleanup and test isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7110734-256e-4cd9-a294-a3d8fe1ffa6b
📒 Files selected for processing (16)
bom/pom.xmlcodec/codec-sofa-fory/pom.xmlcodec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.javacodec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.javacodec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.javacodec/codec-sofa-fory/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.codec.Serializercodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/blacklist/BlackListClass.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/none/NoneClassHasBlackClass.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/whitelist/DemoRequest.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/whitelist/DemoResponse.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/whitelist/DemoService.javacodec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/whitelist/WhiteClassHasBlackClass.javacodec/codec-sofa-fory/src/test/resources/sofa-rpc/serialize_blacklist.txtcodec/codec-sofa-fory/src/test/resources/sofa-rpc/serialize_whitelist.txtcodec/pom.xml
codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java
Outdated
Show resolved
Hide resolved
...-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces a new SOFARPC codec module (codec-sofa-fory) that adds Apache Fory-based serialization as a new Serializer extension (fory, code=23), along with request/response custom serializers and unit tests.
Changes:
- Added new
codec/codec-sofa-forymodule implementingForySerializer+ custom serializers forSofaRequest/SofaResponseand SPI registration. - Added Apache Fory dependency management (
fory.version=0.16.0) in the BOM. - Registered the new module in
codec/pom.xmland added unit tests + test blacklist/whitelist resources for checker mode coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| codec/pom.xml | Adds codec-sofa-fory as a codec submodule. |
| bom/pom.xml | Adds org.apache.fory:fory-core version management via fory.version. |
| codec/codec-sofa-fory/pom.xml | Declares the new codec module and its dependencies. |
| codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java | Implements the new serializer extension and configures Fory + checker modes. |
| codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaRequestForySerializer.java | Custom Fory serializer for SofaRequest (incl. template decode). |
| codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java | Custom Fory serializer for SofaResponse (incl. template decode). |
| codec/codec-sofa-fory/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.codec.Serializer | Registers the fory SPI implementation. |
| codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java | Adds unit tests for encode/decode, template decode, and checker modes. |
| codec/codec-sofa-fory/src/test/resources/sofa-rpc/serialize_whitelist.txt | Test whitelist used by checker-mode tests. |
| codec/codec-sofa-fory/src/test/resources/sofa-rpc/serialize_blacklist.txt | Test blacklist used by checker-mode tests. |
| codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/model/** | Test model classes used to validate whitelist/blacklist behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java
Outdated
Show resolved
Hide resolved
...-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java
Show resolved
Hide resolved
...-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java
Show resolved
Hide resolved
...-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/serialize/SofaResponseForySerializer.java
Show resolved
Hide resolved
codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java
Outdated
Show resolved
Hide resolved
codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java
Outdated
Show resolved
Hide resolved
Collaborator
Code Review 报告整体评价代码结构清晰，与现有 FurySerializer 保持一致的设计模式，测试覆盖较为全面。以下是具体发现： 需要修复的问题1. 缺少
|
| 类型 | 数量 |
|---|---|
| 需要修复 | 3 |
| 建议改进 | 1 |
整体代码质量良好，建议修复上述问题后合并。
- Split decode null/empty checks into separate error messages for clarity - Add SofaRpcException rethrow in encode/decode catch blocks to avoid double-wrapping - Change STRICT mode from else to else-if for explicit conditional logic - Fix SofaResponseForySerializer error types: SERVER_DESERIALIZE->SERVER_SERIALIZE in encode, CLIENT_SERIALIZE->CLIENT_DESERIALIZE in decode - Wrap WARN/DISABLE mode System.setProperty in try-finally to guarantee cleanup - Fix assertEquals parameter order to (expected, actual) in test assertions - Add Javadoc @author annotations to all Fory serializer classes
Collaborator
功能建议：在 tracelog 中记录序列化方式背景当前 PR 新增了 Apache Fory 序列化器（code=23），项目中已有多种序列化方式：
问题查看
但没有记录实际使用的序列化方式，这导致：
建议在 /***
* 序列化类型
*/
public static final String SERIALIZE_TYPE = "serialize.type";在 客户端 ( // 从 request 获取 serializeType 并转换为可读名称
byte serializeType = request.getSerializeType();
String serializeTypeName = SerializerFactory.getSerializerNameByCode(serializeType);
clientSpan.setTag(RpcSpanTags.SERIALIZE_TYPE, serializeTypeName);服务端 ( byte serializeType = request.getSerializeType();
String serializeTypeName = SerializerFactory.getSerializerNameByCode(serializeType);
serverSpan.setTag(RpcSpanTags.SERIALIZE_TYPE, serializeTypeName);预期效果tracelog 中将包含类似： {
"serialize.type": "fory",
"req.serialize.time": 5,
"resp.deserialize.time": 3
}这样可以：
这个建议可以作为后续优化项，不影响当前 PR 合并。 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
codec/codec-sofa-fory/src/main/java/com/alipay/sofa/rpc/codec/fory/ForySerializer.java
Show resolved
Hide resolved
添加了两个新的测试方法用于验证Java与Python之间的跨语言序列化兼容性： - `testXlangReadFromPython`: 测试读取Python生成的二进制数据并反序列化为DemoRequest对象 - `testXlangReadFromJava`: 测试读取预生成的Java二进制数据并反序列化 同时增加了必要的导入语句和构建专用FORY实例的方法。
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java (2)
167-212: AlignbuildRequest()with whatassertEqualsSofaRequest()actually verifies.Lines 178-180 populate
serializeType,timeout, andinvokeType, but the assertion helper never checks them. If those fields stop round-tripping,testSofaRequest()still passes.🧪 Suggested assertions
private void assertEqualsSofaRequest(SofaRequest request, SofaRequest decode) { Assert.assertEquals(request.getInterfaceName(), decode.getInterfaceName()); Assert.assertEquals(request.getMethodName(), decode.getMethodName()); Assert.assertArrayEquals(request.getMethodArgSigs(), decode.getMethodArgSigs()); Assert.assertEquals(request.getMethodArgs().length, decode.getMethodArgs().length); Assert.assertEquals("name", ((DemoRequest) decode.getMethodArgs()[0]).getName()); Assert.assertEquals(request.getTargetServiceUniqueName(), decode.getTargetServiceUniqueName()); Assert.assertEquals(request.getTargetAppName(), decode.getTargetAppName()); + Assert.assertEquals(request.getSerializeType(), decode.getSerializeType()); + Assert.assertEquals(request.getTimeout(), decode.getTimeout()); + Assert.assertEquals(request.getInvokeType(), decode.getInvokeType()); Assert.assertEquals(request.getRequestProp(RemotingConstants.RPC_TRACE_NAME), decode.getRequestProp(RemotingConstants.RPC_TRACE_NAME)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java` around lines 167 - 212, The test sets serializeType, timeout and invokeType in buildRequest() but assertEqualsSofaRequest() doesn't verify them, so add assertions to assertEqualsSofaRequest() to check request.getSerializeType() vs decode.getSerializeType(), request.getTimeout() vs decode.getTimeout(), and request.getInvokeType() vs decode.getInvokeType() (or remove setting of those fields in buildRequest() if they are irrelevant); update the helper method (assertEqualsSofaRequest) to include these checks to ensure those fields round-trip.
353-359: Fix the fixture provenance in this Javadoc.This method loads
fory_xlang_from_java.binand asserts"hello from java", so referencing the Python test here is misleading when someone needs to regenerate or inspect the Java fixture.📝 Suggested wording
- * Cross-language test: Java reads the pre-generated bytes produced by the Python test - * ({`@code` TestForyXlangInterop#test_python_to_java_serialize}) and deserializes them. + * Cross-language test: Java reads the committed + * {`@code` src/test/resources/xlang/fory_xlang_from_java.bin} fixture and deserializes it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java` around lines 353 - 359, Update the Javadoc for the cross-language test in ForySerializerTest to correctly state the fixture provenance: replace the claim that the bytes were produced by the Python test (TestForyXlangInterop#test_python_to_java_serialize) with wording that the test loads the committed bin file for Java (fory_xlang_from_java.bin) and asserts the expected deserialized value ("hello from java"); edit the Javadoc comment above the test method in ForySerializerTest so it correctly documents that the fixture was generated from Java and is stored in src/test/resources/xlang/fory_xlang_from_java.bin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java`:
- Around line 54-55: The test captures a JVM-global property and a serializer
instance at field construction, making testChecker() order-dependent; change
tests to create the ForySerializer instance locally after setting the desired
system property (call
ExtensionLoaderFactory.getExtensionLoader(Serializer.class).getExtension("fory")
inside the test rather than using the field 'serializer'), and wrap the test in
try/finally where you save the original
System.getProperty("sofa.rpc.codec.serialize.checkMode"), set the needed value
(e.g., "STRICT"/"WARN"/"DISABLE"), run assertions, then restore the original
value (if non-null) or remove the property only if it was originally
absent—apply the same local-creation + save/restore pattern to the other
affected tests in ForySerializerTest (the block around lines 215-297).
---
Nitpick comments:
In
`@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java`:
- Around line 167-212: The test sets serializeType, timeout and invokeType in
buildRequest() but assertEqualsSofaRequest() doesn't verify them, so add
assertions to assertEqualsSofaRequest() to check request.getSerializeType() vs
decode.getSerializeType(), request.getTimeout() vs decode.getTimeout(), and
request.getInvokeType() vs decode.getInvokeType() (or remove setting of those
fields in buildRequest() if they are irrelevant); update the helper method
(assertEqualsSofaRequest) to include these checks to ensure those fields
round-trip.
- Around line 353-359: Update the Javadoc for the cross-language test in
ForySerializerTest to correctly state the fixture provenance: replace the claim
that the bytes were produced by the Python test
(TestForyXlangInterop#test_python_to_java_serialize) with wording that the test
loads the committed bin file for Java (fory_xlang_from_java.bin) and asserts the
expected deserialized value ("hello from java"); edit the Javadoc comment above
the test method in ForySerializerTest so it correctly documents that the fixture
was generated from Java and is stored in
src/test/resources/xlang/fory_xlang_from_java.bin.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 158da5ea-611f-4c54-9223-c4348e584999
⛔ Files ignored due to path filters (2)
codec/codec-sofa-fory/src/test/resources/xlang/fory_xlang_from_java.binis excluded by!**/*.bincodec/codec-sofa-fory/src/test/resources/xlang/fory_xlang_from_python.binis excluded by!**/*.bin
📒 Files selected for processing (1)
codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java
Comment on lines
+54
to
+55
| private final ForySerializer serializer = (ForySerializer) ExtensionLoaderFactory.getExtensionLoader( | ||
| Serializer.class).getExtension("fory"); |
Contributor
There was a problem hiding this comment.
Isolate testChecker() from ambient JVM state.
Line 54’s field-created serializer captures the checker mode at construction time, so the first block is only testing whatever sofa.rpc.codec.serialize.checkMode the JVM already had, not necessarily STRICT. The WARN/DISABLE branches then remove the property instead of restoring any previous value, which makes the suite order-dependent.
🔧 Suggested change
`@Test`
public void testChecker() throws Exception {
- // default fory checkMode is STRICT
+ final String key = "sofa.rpc.codec.serialize.checkMode";
+ final String previous = System.getProperty(key);
+
WhiteClassHasBlackClass whiteClassNullBlackClass = new WhiteClassHasBlackClass();
NoneClassHasBlackClass noneClassNullBlackClass = new NoneClassHasBlackClass();
@@
- try {
- serializer.encode(noneClassNullBlackClass, null);
+ try {
+ System.setProperty(key, "STRICT");
+ ForySerializer strictForySerializer = new ForySerializer();
+ try {
+ strictForySerializer.encode(noneClassNullBlackClass, null);
+ Assert.fail();
+ } catch (Exception e) {
+ // expected: NoneClass not in whitelist
+ }
+ ...
+
+ System.setProperty(key, "WARN");
+ ForySerializer warnForySerializer = new ForySerializer();
+ ...
+
+ System.setProperty(key, "DISABLE");
+ ForySerializer disableForySerializer = new ForySerializer();
+ ...
} finally {
- System.getProperties().remove("sofa.rpc.codec.serialize.checkMode");
+ if (previous == null) {
+ System.clearProperty(key);
+ } else {
+ System.setProperty(key, previous);
+ }
}
}Also applies to: 215-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codec/codec-sofa-fory/src/test/java/com/alipay/sofa/rpc/codec/fory/ForySerializerTest.java`
around lines 54 - 55, The test captures a JVM-global property and a serializer
instance at field construction, making testChecker() order-dependent; change
tests to create the ForySerializer instance locally after setting the desired
system property (call
ExtensionLoaderFactory.getExtensionLoader(Serializer.class).getExtension("fory")
inside the test rather than using the field 'serializer'), and wrap the test in
try/finally where you save the original
System.getProperty("sofa.rpc.codec.serialize.checkMode"), set the needed value
(e.g., "STRICT"/"WARN"/"DISABLE"), run assertions, then restore the original
value (if non-null) or remove the property only if it was originally
absent—apply the same local-creation + save/restore pattern to the other
affected tests in ForySerializerTest (the block around lines 215-297).
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements Apache Fory serialization support for SOFARPC, as discussed in #1544.
Changes
New Module:
codec/codec-sofa-foryForySerializer: Main serializer usingorg.apache.fory:fory-core:0.16.0, registered as@Extension(value="fory", code=23)to avoid conflict with the legacyfury2(code=22)SofaRequestForySerializer: Custom serializer forSofaRequestSofaResponseForySerializer: Custom serializer forSofaResponseDISABLE,WARN,STRICT(viaAllowListChecker)META-INF/services/sofa-rpc/com.alipay.sofa.rpc.codec.SerializerUpdated Files
bom/pom.xml: Addedfory.version=0.16.0dependency managementcodec/pom.xml: Addedcodec-sofa-forysubmoduleKey Differences from Legacy FurySerializer
org.furyio:fury-core:0.4.1org.apache.fory:fory-core:0.16.0io.fury.*org.apache.fory.*getClassResolver().setClassChecker()getTypeResolver().setTypeChecker()Testing
All 4 unit tests pass:
encodeAndDecode- basic encode/decodetestSofaRequest- SofaRequest serialization with template decodetestSofaResponse- SofaResponse serialization with template decodetestChecker- STRICT / WARN / DISABLE security check modesCloses #1544
Summary by CodeRabbit
New Features
Tests
Chores