fix: remove unused Platform serialization code path - #648
Conversation
a22f55f to
4570767
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
============================================
+ Coverage 66.03% 66.80% +0.77%
+ Complexity 214 211 -3
============================================
Files 35 35
Lines 998 973 -25
Branches 143 136 -7
============================================
- Hits 659 650 -9
+ Misses 287 272 -15
+ Partials 52 51 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey @maxday! 👋
I agree that simplifying this logic would be valuable.
However, I found a concrete compatibility concern that makes me hesitant to classify this path as unused: the AWS SDK for Android’s Lambda invoker automatically supplies the platform value that selects it.
Specifically:
- ClientContext sets
env.platformto"Android". - LambdaInvokerFactory constructs that client context, and LambdaInvocationHandler attaches it to the invocation.
- LambdaInvokerFactory uses LambdaJsonBinder by default, which uses Gson for request serialization and response deserialization.
This suggests that the runtime’s Gson path was intended to provide serialization semantics compatible with the Android client. I haven’t found an explicit statement confirming that intent. The AWS SDK for Android is now archived, and I couldn’t find this invoker or an equivalent direct-invocation API in the current Amplify Android repository. Existing applications may therefore still depend on the archived SDK and this behavior.
Since this PR changes both input deserialization and output serialization, I think we should treat it as potentially breaking for those callers. If you have additional information showing that this integration has been replaced, is no longer used, or that compatibility is otherwise covered, I’m happy to revisit that assessment.
If we decide to remove this path, I see three possible approaches:
-
Merge this change, remove Gson as a dependency, and release new major versions of both Serialization and the RIC from a separate branch. These versions could initially be used only by OCI-based functions and a future managed Java runtime. However, this would require maintaining the existing versions and backporting changes, so I would not recommend this approach at the moment.
-
Consolidate both paths on Jackson, while configuring and testing Jackson to preserve the serialization behavior expected by Android clients. If we can establish compatibility, we could remove Gson without introducing a breaking change.
-
Follow the migration path I documented last year: use a service-provider design to decouple serialization from the RIC, preserve the existing behavior by default, and allow customers to opt into a new implementation. This could also support a future Jackson major-version upgrade, although I would keep that as a separate design discussion rather than a prerequisite for this PR.
For the last option, separating these concerns internally would not necessarily require publishing every resulting module. In general, I think we should publish fewer libraries to Maven Central, each with a clear customer use case. For example, I don’t see a compelling reason for customers to consume our serialization implementation independently of the RIC.
Overall, I support the cleanup; I’d just like us to understand the compatibility impact and agree on a migration strategy before shipping it.
Issue #, if available:
Description of changes:
Target (OCI, Managed Runtime, both):
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.