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

Add tests for queries with denormalized links #364

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Floriferous
Copy link
Contributor

Queries with denormalized links fail in all sorts of ways, there's a lot that breaks.

I added a couple tests that highlight the kinds of things that fail, and tried to make a change that helps with reducing the issue, but is not nearly good enough.

Basically, combining grapher-caches with reducers and real caches does not work:

// This query
comments: {
  author: { name: 1 },
  authorCache: 1, // There's more data in the cache
}

// Returns this data:
comments: {
  _id: 1,
  authorId: 1,
  author: { } //  full cache data here, should only have name
  // Missing authorCache
}

With my initial fix:

// This query
comments: {
  author: { name: 1 },
  authorCache: 1,
}

// Returns this data:
comments: {
  _id: 1,
 authorId: 1,
 author: { } //  full cache data here, should only have name
 authorCache: { } //  full cache data here, is correct
}

Todo:

  • Fix all the new tests
  • Probably add more denormalization-related tests, since none exist at the moment

@Floriferous Floriferous marked this pull request as ready for review November 4, 2021 12:47
@StorytellerCZ StorytellerCZ added this to the V1.4.1 milestone Oct 6, 2022
@StorytellerCZ
Copy link
Collaborator

@Floriferous would you be so kind to update this to the latest master? Thank you!

@StorytellerCZ
Copy link
Collaborator

Did a merge in my branch, here are the failing tests: https://github.com/StorytellerCZ/grapher/actions/runs/3810463294/jobs/6482555473

@StorytellerCZ StorytellerCZ removed this from the V1.4.1 milestone Dec 31, 2022
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.

2 participants