Skip to content
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

Making GraphQLField's cacheKey method public instead of internal #1972

Closed
potrebic opened this issue Sep 28, 2021 · 12 comments · Fixed by #2014
Closed

Making GraphQLField's cacheKey method public instead of internal #1972

potrebic opened this issue Sep 28, 2021 · 12 comments · Fixed by #2014
Assignees

Comments

@potrebic
Copy link

Feature request

Making GraphQLField's cacheKey method public instead of internal

Motivation

We're using GraphQL and Apollo. I have several ideas which led me to this request. For one I like the idea of being able to log a nice description of a Query - basically matching what the Apollo client itself uses as the Query's key in QUERY_ROOT:

func some(query: GraphQLQuery, field: GraphQLField) {
        let key = try? field.cacheKey(with: query.variables)
        print(key)
}

There are other things I'm exploring related to tracking of queries and then being able to purge queries that haven't been executed within some time window.

Proposed solution

In Sources/Apollo/GraphQLSelectionSet.swift

  public func cacheKey(with variables: [String: JSONEncodable]?) throws -> String { ... }

Outstanding Questions

I can certainly create a PR with this change if it seems appropriate.

@calvincestari
Copy link
Member

Hi @potrebic 👋🏻 - at first glance I can't think of any downside to making cacheKey(variables:) available with public access.

I'm more intrigued by this..

There are other things I'm exploring related to tracking of queries and then being able to purge queries that haven't been executed within some time window.

Are you wanting to purge them from single clients ad hoc or from the whole schema across all clients? If it's across the whole schema then query metrics from your server implementation would be more reliable; that way you'd be able cater for older mobile clients too.

@AnthonyMDev
Copy link
Contributor

I have concerns about making this public. The implementation of the caching layer is going to have a ton of changes to it in the future. For all we know, this method could be returning something very different in a future release. Exposing this information, which is really internal implementation details, opens the doors for people to rely on the behavior of the cache. We aren't making any promises about the internal behavior of the cache to stay the same.

I don't want it to be an API breaking change when we make internal cache improvements. My inclination here is that, if there is specific functionality that is desired as far as interacting with the cache, we should implement it in Apollo and support it going forward.

There are other things I'm exploring related to tracking of queries and then being able to purge queries that haven't been executed within some time window.

These are great examples of things we want the Apollo Client to support out of the box. And once we finish cleaning up the code generation layer and the networking layer, I intend to spend significant time on additional functionality for the cache.

If you would like to work on that type of functionality, my request would be that you do so by making PRs to include it in the Apollo Client. I'll admit that this won't be easy with the current implementation. There is a lot of work that needs to be done on the caching layer to make it more extensible. It's pretty rigid and fragile currently. Which is why exposing those unstable APIs publicly seems like it will put us in a difficult position in the future.

For one I like the idea of being able to log a nice description of a Query - basically matching what the Apollo client itself uses as the Query's key in QUERY_ROOT:

For logging purposes, I recommend you just log the Query Operation by name and log the operation's variable alongside that. But because the way we calculate cache paths is liable to change, I don't feel great about recommending that you log the cache paths specifically.

At some point in the future, the caching APIs may be stable enough that we can expose more of them. That is definitely something I hope for.

@potrebic
Copy link
Author

potrebic commented Oct 4, 2021

@AnthonyMDev @calvincestari

First - thanks for the replies!

If the format of the Cache References changes from release to release... will the Apollo client handle DB migration? Asking because while the format isn't currently public, it is codified in the database. And changing the format (without a DB migration) will render obsolete the local cache.

Our data/DBs will last for years and the customer experience relies on the durability of that data.

My first example, logging doesn't care about the format. Just uniqueness and short term durability (lifetime of Process)


I'm working on a new use case right now where having this public would be "nice". We use the Apollo watch() facility. And on top of that build a publisher change (Apple Combine). This publisher is given out to clients of this code. The one publisher supports multiple subscriptions (using multicast, etc) for memory/efficiency purposes. And of course we want just one Apollo watcher backing this publisher. Mapping an incoming query to its publisher (if one is cached), I'd like to use the SelectionSet's cacheKey method to create the "key". I can of course spin my own logic.

This use case doesn't care about the particulars of the format. Just that it be unique and durable for the lifetime of the Process, which of course it is, since it's way more durable than that (being used as a lookup in the QUERY_ROOT table.)


Here's the more critical need. In our graph not all the data, the parts of the data we want direct access to, is backed by actual calls to the backend GraphQL endpoint. In effect we add query types to the schema. If we just want to "fetch", we can of course use ReadObject. We also want to "watch" these areas of the graph. And that we can't do using ReadObject. And the normal watch doesn't work.

I'm pretty new to Apollo. So I don't at present have a feel for the possibility of being able to "watch an Object". Likely only makes sense for Objects that have semantic, durable names (and 100% of our objects do). We don't have any naming like "edge_0, edge_1, etc".

What I came up with is a kind of query that can be "watched" without actually having an entry in the QUERY_ROOT table. Instead one is fabricated on the fly. Using a custom Cache layer that key (field) is "injected" into the in-memory QUERY_ROOT results before proceeding with the loadRecords() calls.

This use-case doesn't care about the particulars of the key. Just that it be the correct key. Thus, if the format changes this code wouldn't care (though we'd care and our customers would care if all the local data was lost)


In general (and again I'm new to Apollo, so my opinion here might not be well formed) I wonder about the QUERY_ROOT table. Seems like it just grows and grows. And executing any query incurs the cost of loading the entire table, only to in-effect find a mapping from something that is a "constant" (the cacheKey) to something that for queries with normalized names pretty much contains the same information. Maybe this doesn't apply to "all" queries, but it does too many (And 100% of ours). For example, my QR table has:

{
"collection(id:8390094800)":{"$reference":"Collection:8390094800"},
"collection(id:8383345357)":{"$reference":"Collection:8383345357"},
...
}

So I've got a query for 2 "collections". And the reference name has basically the same info as the key. So a Query, perhaps one that implemented a special protocol, announcing itself as this kind of query, could provide the name for it's cache key... then such a query could be a first class citizen (it seems) without having to read the QUERY_ROOT table.

This is basically my implementation by creating a custom interceptor chain, customer local-cache-reader, and a custom Cache which wraps the SQL cache. And it would benefit from having access to the SelectionSet's cacheKey() method.

@designatednerd
Copy link
Contributor

Heads up that @AnthonyMDev is out until later in the week but he'll be getting back to you when he gets back - he's been doing a ton of thinking around all this stuff lately so I'll defer to him in terms of how we're thinking about this going forward

@AnthonyMDev
Copy link
Contributor

I was about to respond and say that I’ll reply in detail when I’m back from vacation. Thanks @designatednerd!

I have a lot of thoughts here, and I’ll give you a run down of our future plans when I’m home (Thursday).

@designatednerd
Copy link
Contributor

@AnthonyMDev GET OUT! You're on vacation!

@AnthonyMDev
Copy link
Contributor

@potrebic Thank you for this run down. I've been doing some thinking about this, and I'm going to be having a meeting today to discuss how we should proceed. Would you be interested in chatting with me a bit more about your problem and the current iteration of your solution? I'd love to set up a Zoom call and get more context from you on this.

@potrebic
Copy link
Author

@AnthonyMDev And now I've been away for a few days (Visiting my son at college!!!). I'd be happy to get together via zoom. Tuesday (ideal) or after would be great. I'm on the west coast (in terms of scheduling).

@AnthonyMDev
Copy link
Contributor

@potrebic Hope you enjoyed your vacation as well! I'm available tomorrow (Tuesday), and I am PST as well. Shoot me an e-mail with times you're available and we'll set something up.

@AnthonyMDev
Copy link
Contributor

After a conversation with @potrebic today, I think we should expose this function publicly for the time being. For the 1.0 release, I'd like to consider if there is a better way to expose cache keys that is safer and more stable, but since we are rewriting a large piece of the execution layer there, exposing this in the 0.x version for now seems fine.

@AnthonyMDev
Copy link
Contributor

@potrebic I'll get this in with the next release. If you want to make the PR so you get the contributor cred, feel free; otherwise I'll handle it.

@AnthonyMDev
Copy link
Contributor

Release 0.50.0 is out! @potrebic hope that makes your life easier!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants