Skip to content

Fix value comparison, argument merging, and link feature addition/extraction in composition - #3134

Merged
sachindshinde merged 2 commits into
mainfrom
sachin/cost-tweaks
Sep 14, 2024
Merged

Fix value comparison, argument merging, and link feature addition/extraction in composition#3134
sachindshinde merged 2 commits into
mainfrom
sachin/cost-tweaks

Conversation

@sachindshinde

@sachindshinde sachindshinde commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

While reviewing recent code changes that released in 2.9.0, I noticed a few bugs in the code, so I've pushed some fixes for them in this PR (along with some fixes for more ancient bugs).

I'll comment on the code with more details, but to summarize the fixes:

  • Value comparison/default value application has been updated to handle nulls/undefineds better.
  • Composition argument strategy code has been refactored to handle nullability more cleanly (and fix some bugs around nullability).
    • Note I've commented out the unused SUM strategy, as using it currently wouldn't yield expected results since directive applications are deduped before being merged.
  • The logic that applies @link applications to the supergraph schema for supergraph specs needed by subgraph directives has been updated to handle specs that have multiple directives that need composing (the behavior before was adding spurious directive definitions to the supergraph schema).
  • Some ancient logic around computing directive/type names for @core/@link was off.

@sachindshinde
sachindshinde requested a review from a team as a code owner August 30, 2024 19:52
@changeset-bot

changeset-bot Bot commented Aug 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2e54f98

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@apollo/federation-internals Patch
@apollo/gateway Patch
@apollo/composition Patch
@apollo/query-planner Patch
@apollo/query-graphs Patch
@apollo/subgraph Patch
apollo-federation-integration-testsuite Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify

netlify Bot commented Aug 30, 2024

Copy link
Copy Markdown

Deploy Preview for apollo-federation-docs canceled.

Name Link
🔨 Latest commit 2e54f98
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/66e4c9aa208dec0008b83263

@codesandbox-ci

codesandbox-ci Bot commented Aug 30, 2024

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@sachindshinde sachindshinde changed the title Miscellaneous bugfixes Miscellaneous bugfixes for 2.9 Aug 31, 2024
@sachindshinde
sachindshinde force-pushed the sachin/cost-tweaks branch 2 times, most recently from 9fac996 to babcdc4 Compare September 3, 2024 20:05
@sachindshinde sachindshinde changed the title Miscellaneous bugfixes for 2.9 Composition bugfixes for 2.9 Sep 3, 2024
@sachindshinde sachindshinde changed the title Composition bugfixes for 2.9 Fix value comparison, argument merging, and link feature addition/extraction in composition Sep 3, 2024
@sachindshinde
sachindshinde merged commit e6c05b6 into main Sep 14, 2024
@sachindshinde
sachindshinde deleted the sachin/cost-tweaks branch September 14, 2024 00:02
sachindshinde added a commit that referenced this pull request Sep 17, 2024
sachindshinde added a commit that referenced this pull request Sep 17, 2024
sachindshinde added a commit that referenced this pull request Sep 17, 2024
The PRs #3134 and #3136 were merged without tests illustrating the bugs
they were fixing. This PR adds tests for two of the bugs fixed by them;
these tests fail on `2.9.0`, but succeed after the aforementioned PRs.
sachindshinde pushed a commit that referenced this pull request Sep 18, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/composition@2.9.1

### Patch Changes

- Fix bugs in composition when merging nulls in directive applications
and when handling renames.
([#3134](#3134))

- Updated dependencies
\[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5),
[`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]:
    -   @apollo/federation-internals@2.9.1
    -   @apollo/query-graphs@2.9.1

## @apollo/gateway@2.9.1

### Patch Changes

- Fix bugs in composition when merging nulls in directive applications
and when handling renames.
([#3134](#3134))

- Updated dependencies
\[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5),
[`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]:
    -   @apollo/federation-internals@2.9.1
    -   @apollo/composition@2.9.1
    -   @apollo/query-planner@2.9.1

## @apollo/federation-internals@2.9.1

### Patch Changes

- Fix edge cases for subgraph extraction logic when using spec renaming
or specs URLs that look similar to `specs.apollo.dev`.
([#3136](#3136))

- Fix bugs in composition when merging nulls in directive applications
and when handling renames.
([#3134](#3134))

## @apollo/query-graphs@2.9.1

### Patch Changes

- Updated dependencies
\[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5),
[`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]:
    -   @apollo/federation-internals@2.9.1

## @apollo/query-planner@2.9.1

### Patch Changes

- Updated dependencies
\[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5),
[`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]:
    -   @apollo/federation-internals@2.9.1
    -   @apollo/query-graphs@2.9.1

## @apollo/subgraph@2.9.1

### Patch Changes

- Updated dependencies
\[[`b8e4ab5352a4dfd262af49493fdd42e86e5e3d99`](b8e4ab5),
[`e6c05b6c96023aa3dec79889431f8217fcb3806d`](e6c05b6)]:
    -   @apollo/federation-internals@2.9.1

## apollo-federation-integration-testsuite@2.9.1

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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