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

Use _bramble__typename alias for bramble injected __typename #105

Merged
merged 3 commits into from
Nov 9, 2021

Conversation

lucianjon
Copy link
Contributor

Following on from the work in #94. With that change in place there are certain aliases that are reserved for bramble's internal use, this changes the __typename alias to be _bramble__typename.

Rationale

  • create a pattern of reserved aliases starting with _bramble
  • reduce the chance of someone wanting to genuinely use the __typename alias for different purposes being unable to do so
  • because of the point above there should generally be no confusion about whether the client requested the __typename field or bramble injected it

As a side note, any bramble injected fields (that were also not explicitly requested) are automatically omitted from the final response. This is because the response formatting uses the client's selection set to decide what goes into the response. Also, in the future we could consider failing queries that include the reserved aliases entirely.

New Test Suite

On top of the above changes there is a new minimal test suite, designed to test much more of bramble's internals in one go. The main difference between these tests and the tests in execution_test.go is the new suite will bring up and query real graphql servers. The tests are in server_test.go and the test server implementations in a package testsrv.

Given these servers are not mocked out there is a high overhead to new schemas and changes to what they now have. So the tests against them are minimal for now.

The biggest benefit of these tests is testing the plan against spec compliant graphql servers. The existing tests rely on manually crafted JSON to use as graphql responses and so don't test the plan itself but rather what the person writing the tests thinks it should be. Manually writing JSON is also fairly error prone and can cause frustrating test failures.

They also give us a quick end to end test that makes sure bramble can fetch schemas of downstream services, merge these and then run queries on the merged schema.

@lucianjon lucianjon requested a review from a team as a code owner November 9, 2021 21:01
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 idea with the test suite, might be able to make the test servers skip the network stack if we abstract out the Service like we've been talking about for v2.

import (
"testing"

"github.com/movio/bramble/testsrv"
Copy link
Contributor

Choose a reason for hiding this comment

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

High-five on the end-to-end tests! I was nervous about those being missing while updating test fixtures. Nice work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yeah definitely more comfortable making changes in the planner/execution pipeline with real servers to check against

@lucianjon lucianjon merged commit e0e24cc into movio:main Nov 9, 2021
@lucianjon lucianjon deleted the use_bramble_typename branch November 9, 2021 22:59
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.

None yet

3 participants