-
Notifications
You must be signed in to change notification settings - Fork 660
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
Add Factory to create NormalizedCaches injected with Scalar adapters #405
Conversation
the CustomScalarType serializers.
959cb12
to
ddf8946
Compare
* An adapter used to serialize and deserialize Record fields. Record object types will be serialized to | ||
* {@link CacheReference}. | ||
*/ | ||
public final class RecordFieldAdapter { |
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.
Should we move this class out from /sql
package as it's seems more generic and not specific to sql
import javax.annotation.Nonnull; | ||
import javax.annotation.Nullable; | ||
|
||
public abstract class CacheStore { |
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.
Not sure if we need to call it ApolloCacheStore
?
EvictionPolicy evictionPolicy, | ||
Optional<NormalizedCacheFactory> secondaryNormalizedCache) { | ||
super(recordFieldAdapter); | ||
if (secondaryNormalizedCache.isPresent()) { |
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.
nit: I guess it would be better:
this.secondaryCache = secondaryNormalizedCache.transform(it -> it.createNormalizedCache(recordFieldAdapter)));
@@ -70,8 +86,8 @@ public LruNormalizedCache(EvictionPolicy evictionPolicy, NormalizedCache seconda | |||
} | |||
|
|||
@Nonnull @Override public Set<String> merge(Record apolloRecord) { | |||
if (secondaryCacheStore.isPresent()) { | |||
secondaryCacheStore.get().merge(apolloRecord); | |||
if (secondaryCache.isPresent()) { |
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.
I wish we selected Java8 Optional implementation for internal use.
this can be replaced with Java8 optional:
secondaryCache.ifPresent(it -> it.merge(apolloRecord))
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.
Looks cool!
closes #401
Using a factory pattern to manage the complexity of making sure that normalized cache implementations are injected with the same scalar adapter set as is set in the
ApolloClientBuilder
.