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

Fix SQLite Cache Key Bug #991

Merged
merged 5 commits into from
Feb 10, 2020
Merged

Conversation

giantramen
Copy link

@giantramen giantramen commented Feb 3, 2020

If your query has "." in it this would
create an undefined cache key because it would
just drop the query after the ".". For all "_ROOT" queries
we just want the first part anyways so just return that.

For example here is our query:
QUERY_ROOT.searchPois([criteria:[area:[northeastCorner:latitude:31.882253225338154,longitude:-75.24958693102437],[southwestCorner:latitude:18.87160867771246,longitude:-84.49958923798633]],types:["SITE"]])
And we normally want "QUERY_ROOT" as the key but with the old logic we were getting
"QUERY_ROOT.searchPois([criteria:[area:[northeastCorner:latitude:31.882253225338154,longitude:-75.24958693102437],[southwestCorner:latitude:18.87160867771246,longitude:-84"

If there are more elegant solutions I'm all ears, we were running into issues with this so fixed it the easier way I could find :)

If your query has "." in it this would
create an undefined cache key because it would
just drop the query after the ".". For all "_ROOT" queries
we just want the first part anyways so just return that.
@apollo-cla
Copy link

@giantramen: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@designatednerd
Copy link
Contributor

Looks like we've got some test failures - do you mind also adding an explicit test around this change?

Kamin, Grant added 2 commits February 3, 2020 15:16
Check if the separator is being used in a
float and if it is don't treat it as a separator.
Add test with query for starship length
to test SQL cache keys with floats.
guard let data = graphQLResult.data else {
XCTFail("No data returned with result")
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use let data = try XCTUnwrap(graphQLResult.data) here and not have to deal with writing your own failure mechanism

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but I don't think I can use that in the completion block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had an issue with that in other tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in completion blocks anywhere else, it seems like this test file uses the completion block with XCTFail pattern for all the tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries i'll fight with this once this is merged

@designatednerd
Copy link
Contributor

Test failures seem to be tied to #993 - going to put this into a holding pattern for a minute until we can figure out what's going on there.

@giantramen
Copy link
Author

I am going to take another look at this as well, the changes that were made during review have caused the issue to reappear.

Updated loadingQueryWithFloats test with more
complicated case that matches the original issue
I was seeing.
@giantramen
Copy link
Author

I am going to take another look at this as well, the changes that were made during review have caused the issue to reappear.

Okay this test case actually matches the original issue so should be good now 👍

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna go ahead and merge this one - the failures we're seeing are still tied to #993, so release will be delayed until I figure that nonsense out, but I'm good with the underlying changes with the tests.

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 this pull request may close these issues.

3 participants