-
Notifications
You must be signed in to change notification settings - Fork 396
Add a compile-time switch for RPC enhanced classes #10386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -530,7 +530,7 @@ private void writeClassDeserializationStatements() { | |
| * server, store additional server-only field data using {@link WeakMapping} | ||
| * . | ||
| */ | ||
| if (serializableClass.isEnhanced()) { | ||
| if (Shared.isEnhancedClass(context.getPropertyOracle(), serializableClass)) { | ||
| sourceWriter.println(WEAK_MAPPING_CLASS_NAME + ".set(instance, " + "\"server-enhanced-data-" | ||
| + getDepth(serializableClass) + "\", streamReader.readString());"); | ||
| } | ||
|
|
@@ -580,7 +580,7 @@ private void writeClassSerializationStatements() { | |
| * {@link WeakMapping}. | ||
| */ | ||
|
|
||
| if (serializableClass.isEnhanced()) { | ||
| if (Shared.isEnhancedClass(context.getPropertyOracle(), serializableClass)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems very late to check this - could we instead just not set isEnhanced as you've already done so that this check fails?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I removed the late property checks and restored the direct |
||
| sourceWriter.println("streamWriter.writeString((String) " + WEAK_MAPPING_CLASS_NAME | ||
| + ".get(instance, \"server-enhanced-data-" + getDepth(serializableClass) + "\"));"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,12 @@ class Shared { | |
| */ | ||
| public static final String RPC_ENHANCED_CLASSES = "rpc.enhancedClasses"; | ||
|
|
||
| /** | ||
| * Single-valued configuration property used to disable all enhanced class handling at compile | ||
| * time. | ||
| */ | ||
| public static final String RPC_ENHANCED_CLASSES_ENABLED = "rpc.enhancedClasses.enabled"; | ||
|
|
||
| /** | ||
| * Capitalizes a name. | ||
| * | ||
|
|
@@ -80,6 +86,32 @@ static Set<String> getEnhancedTypes(PropertyOracle propertyOracle) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether RPC should generate support for server-enhanced classes. | ||
| * | ||
| * @param propertyOracle the property oracle used to access the relevant configuration property | ||
| * @return whether enhanced class handling is enabled | ||
| */ | ||
| static boolean shouldEnableEnhancedClasses(PropertyOracle propertyOracle) { | ||
| try { | ||
| ConfigurationProperty prop = | ||
| propertyOracle.getConfigurationProperty(RPC_ENHANCED_CLASSES_ENABLED); | ||
| if (prop.getValues().size() == 1) { | ||
| return Boolean.parseBoolean(prop.getValues().get(0)); | ||
| } | ||
|
niloc132 marked this conversation as resolved.
|
||
| } catch (BadPropertyValueException e) { | ||
| // Preserve the historical behavior when compiling without the new property. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this, given that you also changed RemoteService.gwt.xml?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed—the inherited module normally guarantees the property. I kept only a warned, backwards-compatible fallback for custom modules that replace or do not inherit |
||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether the type should be treated as enhanced for the current compilation. | ||
| */ | ||
| static boolean isEnhancedClass(PropertyOracle propertyOracle, JClassType type) { | ||
| return shouldEnableEnhancedClasses(propertyOracle) && type.isEnhanced(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do need this, contrary to my comment in FieldSerializerCreator, let's swap the && and do the cheap field check before the map lookups
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed |
||
| } | ||
|
|
||
| static String getStreamReadMethodNameFor(JType type) { | ||
| return "read" + getCallSuffix(type); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe tie this back to the actual server behavior being lost through this?
Something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the module documentation to make the server-side consequence explicit: when disabled, the server cannot see the JPA/JDO fields returned to it from client calls.