Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Avoiding empty blocks #29

Merged
merged 1 commit into from
Sep 1, 2018
Merged

Conversation

mrchass
Copy link
Contributor

@mrchass mrchass commented Aug 30, 2018

Hello!
It leads to syntax errors if there are empty blocks '{ }' in graphql requests. There will be an empty block in your request if you create a mutation with no response defined. In this case you have one property (mutation's name) with several arguments but no children.
For example:
An empty class defining the mutation request:
@GraphQLProperty(name="myMutation", arguments={ @GraphQLArgument(name="myArgument1"), @GraphQLArgument(name="myArgument2") }) public class MyMutationModel {}

Creating the request:
GraphQLRequestEntity requestEntity = GraphQLRequestEntity.Builder() .url("http://graphql.example.com/graphql"). .arguments(new Arguments("myMutation", new Argument("myArgument1", "value1"), new Argument("myArgument2", "value2"))) .request(MyMutationModel.class) .build(); GraphQLResponseEntity<Boolean> responseEntity = graphQLTemplate.query(requestEntity, Boolean.class);

The created request would look like this:
mutation {myMutation(myArgument1:"value1", myArgument2:"value2") { } }

But a vlid request has to look like this:

mutation {myMutation(myArgument1:"value1", myArgument2:"value2") }

I think it would be a good idea to prevent empty blocks from being created in general. Because Property.class' member variable "children" is an empty map, at least in this case, it is very likely that empty blocks will be created.

My little modification would fix this behaviour. Or is there another way to deal with requests like this?

Cheers,
Christian

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 43

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.71%

Totals Coverage Status
Change from base Build 28: 0.0%
Covered Lines: 459
Relevant Lines: 465

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 43

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.71%

Totals Coverage Status
Change from base Build 28: 0.0%
Covered Lines: 459
Relevant Lines: 465

💛 - Coveralls

@chemdrew chemdrew merged commit 90ae802 into americanexpress:master Sep 1, 2018
@chemdrew
Copy link
Contributor

chemdrew commented Sep 1, 2018

Looks great, thanks for this contribution!

@mrchass
Copy link
Contributor Author

mrchass commented Sep 3, 2018

@chemdrew : Thank you for accepting this change! Could you tell me, when the next verson will be released and will be available in the repository?
Thanks, Christian

@chemdrew
Copy link
Contributor

chemdrew commented Sep 3, 2018

@mrchass just released v0.1.2 the fix should be available now!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants