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

Configure client generate parser options #1586

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

mgilbey
Copy link
Contributor

@mgilbey mgilbey commented Nov 1, 2022

📝 Description

Allows a user of the client generator to set the parser options for graphql-java. The base generateClient() function accepts a function type with receiver to customise the builder object, whilst the gradle and maven plugins provide an explicit list of (current) options permitted by graphql-java, with the exception of the parsingListener.

🔗 Related Issues

Resolves #1526

Notes

  • I've picked fairly simple naming, but if we suspect any ambiguity of uses, or future collisions, it would be good to call that out so that it's clear this is configuration for the client generation only.
  • Rather than abstract over ParserOptions.Builder the client module now exposes this in it's public API, and would therefore inherit breaking changes made to this class.

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
@mgilbey mgilbey added type: enhancement New feature or request changes: minor Changes require a minor version type: documentation Documentation or test changes module: client Issue affects the client code module: plugin Issue affects the plugins code labels Nov 1, 2022

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
@mgilbey mgilbey added the status: do not merge Do not merge until this is removed label Nov 1, 2022
Copy link
Collaborator

@dariuszkuc dariuszkuc left a comment

Choose a reason for hiding this comment

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

Looks good, just few nitpicks about the docs and the defaults.

@@ -263,6 +289,7 @@ resulting generated code will be automatically added to the project main source
| `serializer` | GraphQLSerializer | | JSON serializer that will be used to generate the data classes.<br/>**Default value is:** `GraphQLSerializer.JACKSON`. |
| `schemaFile` | File | yes | GraphQL schema file that will be used to generate client code. |
| `useOptionalInputWrapper` | Boolean | | Boolean opt-in flag to wrap nullable arguments in `OptionalInput` that distinguish between `null` and undefined/omitted value.<br/>**Default value is:** `false`.<br/>**Command line property is**: `useOptionalInputWrapper` |
| `parserOptions` | GraphQLParserOptions | | Configures the settings used when parsing GraphQL queries and schema definition language documents. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's keep the alphabetical order -> can you also fix serializer <-> schemaFile order?

*this comment applies to all doc changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we also specify somewhere in the docs what are the available options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the ordering and added the specific options and defaults in the docs.

* Configure options for parsing GraphQL queries and schema definition language documents. Settings
* here override the defaults set by GraphQL Java.
*/
data class GraphQLParserOptions(
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of null, should those values have some defaults? i.e. when configuring the plugin I have no idea what would be the default, I have to look up the graphql-java code to find out the defaults

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've set default values that match current graphql-java defaults. They are not particularly accessible in the plugin sources, and if graphql-java changes these we will no longer inherit their defaults. This is easy to reverse in the future if we need to.

@dariuszkuc
Copy link
Collaborator

Rather than abstract over ParserOptions.Builder the client module now exposes this in it's public API, and would therefore inherit breaking changes made to this class.

Client generator module is not intended to be called outside of the maven/gradle plugin which abstract it away so I think it is fine. Unsure if we need to be able to configure all options but why not.

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
…for maven plugin
@mgilbey mgilbey removed the status: do not merge Do not merge until this is removed label Nov 2, 2022
*/
data class GraphQLParserOptions(
/** Modify the maximum number of tokens read to prevent processing extremely large queries */
var maxTokens: Int? = 15000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

if those have default values set -> does it make sense to keep those fields as nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wish to go back to the default value (whatever it is) then you can explicitly set it as null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems odd experience for the end user - if the intent is to keep null as not-set and use defaults from graphql-java lets revert back to keep those values null and document the behavior in the docs

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
…om graphql-java
@samuelAndalon samuelAndalon merged commit 325a6c7 into ExpediaGroup:master Nov 4, 2022
samuelAndalon pushed a commit that referenced this pull request Nov 7, 2022

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
* Configure client generate parser options

* Fix star import failure from ktlint

* Set defaults, update documentation, fix name of parser options class for maven plugin

* Do not set default for parser options, use null to inherit default from graphql-java
dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Nov 7, 2022

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
…Group#1592)

* Configure client generate parser options

* Fix star import failure from ktlint

* Set defaults, update documentation, fix name of parser options class for maven plugin

* Do not set default for parser options, use null to inherit default from graphql-java
samuelAndalon pushed a commit that referenced this pull request Feb 14, 2024

Verified

This commit was signed with the committer’s verified signature.
sandhose Quentin Gliech
…#1925)

Enables configuration of the maxCharacters and maxParserDepth parser
options for GraphQL Java.

### 📝 Description

See
[ParserOptions](https://github.com/graphql-java/graphql-java/blob/7c381cc9d61c1e1838a2487d9b24974c451f23a2/src/main/java/graphql/parser/ParserOptions.java#L13)
for the values configured in graphql-java.

### 🔗 Related Issues

Extensions of #1586 with additional parameters that have been added to
graphql-java.
@mgilbey mgilbey deleted the clientParserOptions branch February 16, 2024 09:59
samuelAndalon pushed a commit that referenced this pull request Feb 22, 2024
… configurable (#1930)

### 📝 Description

Previous val declaration for the additional parser options cannot be
reassigned - they should be a var and not val.

### 🔗 Related Issues

#1925
#1586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: minor Changes require a minor version module: client Issue affects the client code module: plugin Issue affects the plugins code type: documentation Documentation or test changes type: enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Build plugins should allow configuring max parse tokens
3 participants