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

feat(QueryAST marshaller/unmarshaller)!: make ObjectValue a scalar #566

Conversation

xsoufiane
Copy link

please review 🙏

Motivation

This PR is part of the series that aims to make Apollo federation possible in Sangria, and it comes as a response to the json as scalar support, federation issue discussed here.

Changes

This PR implements the new contract described in the testkit PR, by ensuring that QueryAST ObjectValues are GraphQL scalars.

Consequences

  • By accepting these changes, any sangria application using the input object notation instead of variables will be able to accept ObjectValues as scalars. In particular, let's assume the following schema :
    scalar _Any
    type Query {
      _entities(representations: [_Any!]!): [_Entity]!
    } 
    
    with these changes, we can execute these kind of queries (see how an ObjectValue is passed as a scalar):
    {
      "query": "query([
        {
           "__typename": "Product",
           "upc": "B00005N5PF"
        }]) {...}"
    }
    
  • As for the risks, they are the same as discussed [here]((feat(MarshallingBehaviour)!: allow map to be a scalar sangria-marshalling-testkit#22). Basically, extra care must be taking on the application side to ensure that the scalar json objects are not misused.

Notes for reviewer

At the current moment, tests fail because the PR still requires a new release of the testkit, incorporating the changes proposed here. Once, the new release has been published, another commit should be done updating the build with the new version.

@xsoufiane
Copy link
Author

me and @yanns found a way to make federation possible for Sangria without having to change the marshallers and the root sangria lib, and without breaking the existing security level for everyone:) the implementation is done here

@xsoufiane xsoufiane closed this Feb 17, 2021
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