Fix immutable collection or map fields deserialization issue. #260
Conversation
This file contains 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
…sageFactories accept check for IncrementalIdStrategy.
|
@dyu Would you help to review this PR? The change is small and focused, and I have added test cases to verify the specific issue. Many thanks. |
dyu
reviewed
Jan 29, 2019
| } | ||
| catch (IllegalArgumentException e) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
an expensive way to check if it exists ... if there's no other way, then this will probably do
There was a problem hiding this comment.
Indeed. Iterating the enums is probably heavy. How about caching it as static?
public static boolean accept(String name)
{
return MESSAGE_FACTORIES_NAMES.contains(name);
}
static Set<String> MESSAGE_FACTORIES_NAMES;
static
{
MessageFactories[] messageFactories = MessageFactories.values();
MESSAGE_FACTORIES_NAMES = new HashSet<String>(messageFactories.length);
for (MessageFactories messageFactory : messageFactories) {
MESSAGE_FACTORIES_NAMES.add(messageFactory.name());
}
}
There was a problem hiding this comment.
I have updated the PR by using a less expensive way. Please check.
…cept less expensive by caching enum names.
…cept less expensive by caching enum names.
dyu
reviewed
Jan 29, 2019
| * This is used by {@link MessageFactories#accept(String)} method. Rather than iterating enums in runtime | ||
| * which can be an expensive way to do, caching all the enums as static property will be a good way. | ||
| */ | ||
| static Set<String> MESSAGE_FACTORIES_NAMES; |
dyu
reviewed
Jan 29, 2019
| * This is used by {@link MapSchema.MessageFactories#accept(String)} method. Rather than iterating enums in runtime | ||
| * which can be an expensive way to do, caching all the enums as static property will be a good way. | ||
| */ | ||
| static Set<String> MESSAGE_FACTORIES_NAMES; |
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.
What changes were proposed in this pull request?
Immutable collection or map fields usually does not support operations like add/set/remove, for example
java.util.HashMap$KeySet,com.google.common.collect.ImmutableCollection, one solution is to createDelegatefor the specific type to do custommergeFromandwriteToaction.In
DefaultIdStrategyclass, theregisterDelegatemethod relies ontypeClass()provided byDelegateto get the class name.But in some cases where class name can not be initiated like
java.util.HashMap$KeySet, because the inner class is not visible since the class modifier is default.So there should be a way to set specific class name manually. This PR add another
registerDelegatemethod for people to specify class name.Moreover, this PR will solve the issue #227. If one collection is not inlined and cannot be found by
CollectionSchema$MessageFactories, the following exception will be thrown.This PR also solves this issue by checking whether the collection or map class can be found in
CollectionSchema$MessageFactoriesorMapSchema$MessageFactories, if not the collection id or map id will use the full class name instead of simple class name.How was this patch tested?
Add two test cases in
AbstractRuntimeObjectSchemaTestinprotosutff-runtimemodule.One to check
java.util.HashMap$KeySetcan be ser and deser successfully.One to check customized immutable list can be ser and deser successfully.