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: merge composite scopes in final selection #104

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

gmac
Copy link
Contributor

@gmac gmac commented Nov 9, 2021

Fixes a bug with the final filtering of non-distinct composite field selections, per #87.

The Problem

Multiple selection scopes applied to the same object selection were not getting combined in the final result, for example:

query {
  shop1 {
    products {
      id
    }
    products {
      name
    }
  }
}

While this query properly fetched all requested data from underlying services (i.e.: query planning worked as expected), the resulting data only included fields for the first distinct products selection scope:

{
  "data": {
    "shop1": {
      "products": [
        {
          "id": "gid://shopapp/Product/7"
          // ack, no "name"
        }
      ]
    }
  }
}

The Fix

This adjusts unionAndTrimSelectionSetRec, where the mapping of "seenFields" was filtering all repeated selection names as though they were leaf values. This new implementation has repeat composite fields merge their selections together.

Resolves #87.

@gmac gmac requested a review from a team as a code owner November 9, 2021 14:34
@codecov-commenter
Copy link

Codecov Report

Merging #104 (188c1c1) into main (d2cc7fd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   67.50%   67.49%   -0.02%     
==========================================
  Files          24       24              
  Lines        2736     2738       +2     
==========================================
+ Hits         1847     1848       +1     
- Misses        739      740       +1     
  Partials      150      150              
Impacted Files Coverage Δ
query_execution.go 69.83% <100.00%> (+0.12%) ⬆️
auth.go 86.79% <0.00%> (-0.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2cc7fd...188c1c1. Read the comment docs.

@gmac gmac force-pushed the fix-selection-union branch from 188c1c1 to 220985f Compare November 9, 2021 14:38
Copy link
Member

@pkqk pkqk left a comment

Choose a reason for hiding this comment

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

Good find

@lucianjon
Copy link
Contributor

Wicked, thanks @gmac

@lucianjon lucianjon merged commit 0df917f into movio:main Nov 9, 2021
@gmac gmac deleted the fix-selection-union branch November 10, 2021 01:49
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.

Repeated selections for distinct field names are dropped during merge
4 participants