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 defaulting in router config #4010

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Fix defaulting in router config #4010

merged 1 commit into from
Oct 11, 2023

Conversation

BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Oct 10, 2023

There are two types of serde defaulting:

  • container
  • field

Container level defaulting will use an instance of the default implementation and take missing fields from it. Field level defaulting uses either the default implementation or optionally user supplied function to initialize missing fields.

When using field level defaulting it is essential that the Default implementation of a struct exactly match the serde annotations.

A test checks to ensure that field level defaulting is not used in conjunction with a Default implementation and gives guidance on resolution.

Description here

Fixes #4000


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

@github-actions
Copy link
Contributor

@BrynCooke, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@BrynCooke BrynCooke linked an issue Oct 10, 2023 that may be closed by this pull request
@router-perf
Copy link

router-perf bot commented Oct 10, 2023

CI performance tests

  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • large-request - Stress test with a 1 MB request payload
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • reload - Reload test over a long period of time at a constant rate of users
  • no-graphos - Basic stress test, no GraphOS.
  • xxlarge-request - Stress test with 100 MB request payload
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • const - Basic stress test that runs with a constant number of users

@BrynCooke BrynCooke force-pushed the bryn/trace-defaults2 branch from 07cd323 to b86a209 Compare October 10, 2023 12:10
@BrynCooke BrynCooke mentioned this pull request Oct 10, 2023
6 tasks
@BrynCooke BrynCooke force-pushed the bryn/trace-defaults2 branch from b86a209 to 51266d0 Compare October 10, 2023 14:04
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a99485caec5e9d587a3780cd6ed06a546f9924071315ab7280e148536e7ab148"
dependencies = [
"quote 0.6.13",
Copy link
Contributor

Choose a reason for hiding this comment

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

It’d be nice if this new crate can use version 1.0 of quote and syn, to avoid compiling multiple versions of those libraries. But this isn’t a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this, release 0.1.1 and renovate can pick it up later.

apollo-router/Cargo.toml Show resolved Hide resolved
@BrynCooke BrynCooke merged commit cf4a79a into dev Oct 11, 2023
2 checks passed
@BrynCooke BrynCooke deleted the bryn/trace-defaults2 branch October 11, 2023 08:12
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.

Configuration defaults do not match serde
2 participants