Skip to content

Conversation

@dondonz
Copy link
Member

@dondonz dondonz commented Nov 26, 2022

This PR makes String parseValue coercion consistent with the JS reference implementation, by only allowing String input values. Fixes #2990.

Consistent implementation of String coercion is particularly important for String-based custom scalars.

Note that this is a breaking change.

parseValue in JS implementation: https://github.com/graphql/graphql-js/blob/main/src/type/scalars.ts#L156


This PR also fixes the Gradle settings to avoid redirecting to the defunct JCenter when the Gradle module metadata is not published by a plugin (in our case, the jmh plugin). gradle/gradle#22864 (comment)

There was a problem with a JCenter SSL certificate expiring earlier today.

@dondonz dondonz added this to the 20.0 milestone Nov 26, 2022
@dondonz dondonz added the breaking change requires a new major version to be relased label Nov 26, 2022
def "String serialize #value into #result (#result.class)"() {
expect:
Scalars.GraphQLString.getCoercing().serialize(value, GraphQLContext.default, Locale.default) == result
Scalars.GraphQLString.getCoercing().parseValue(value, GraphQLContext.default, Locale.default) == result
Copy link
Member Author

Choose a reason for hiding this comment

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

Splitting out tests for serialize and parseValue

@dondonz dondonz changed the title Make String parseValue coercion consistent with JS implementation Make String parseValue coercion consistent with JS implementation & Gradle JCenter fix Nov 26, 2022
pluginManagement {
repositories {
mavenCentral()
maven {
Copy link
Member Author

Choose a reason for hiding this comment

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

@federicorispo
Copy link
Contributor

@dondonz Since this is a breaking change I would consider to align also the other types:

I can help with these if you like

@dondonz
Copy link
Member Author

dondonz commented Nov 29, 2022

@dondonz Since this is a breaking change I would consider to align also the other types:

Thanks, that's a good pickup @federicorispo. I'll add the other parseValue changes in the next PR

@dondonz dondonz merged commit 8d7c2ba into master Nov 29, 2022
@dondonz dondonz deleted the string-coercion-alignment branch November 29, 2022 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change requires a new major version to be relased

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix GraphQL String coercion

4 participants