Adding Locale to Coercing and hence ValueResolver by bbakerman · Pull Request #2912 · graphql-java/graphql-java

bbakerman · 2022-08-05T03:46:31Z

We would like to support Locale when coer-ercing values or parsing document text. So we need that passed in.

This has chosen to pass in a GraphQLContext and a Locale to a scalar Coercing so that :

a Locale can be present
the Scalar can use the context object for other purposes if it wants to

There are stacks of time where need to use ValueResolver and its methods to resolve values. We need a defaulted locale here and default GraphqlContext to make this work.

The ValueResolver has been substantially re-arranged - none of its logic has changed but the graphql context and locale have been passed on through it.

The class was 1000 lines long. Why too much code in it.

I have split it into 2 - one with the legacy value code into its own class (and then accessed via the ValueResolver) and one with code for value conversions (external to literal etc...)

But the code itself really has not changed. The tests show that - only graphql context and locale have been added

Originally I put the Parser code here (passing in Locale to it) in this PR but it was too big - this is plenty big enough already - so I will make another PR for that

dondonz · 2022-08-08T04:17:43Z

src/main/java/graphql/schema/CoercingEnvironment.java

+    /**
+     * Allows you to build a new coercing environment
+     *
+     * @param <T> for two


dondonz · 2022-08-08T04:46:55Z

src/main/java/graphql/execution/ValuesResolver.java

-                                                   ValueMode valueMode) {
+                                                          GraphQLInputObjectType inputObjectType,
+                                                          Object inputValue,
+                                                          ValueMode valueMode, Locale locale) {


Super nit: add new line before Locale

dondonz · 2022-08-08T04:47:08Z

src/main/java/graphql/execution/ValuesResolver.java

-                                                                      List<VariableDefinition> variableDefinitions,
-                                                                      RawVariables rawVariables) {
+                                                                             List<VariableDefinition> variableDefinitions,
+                                                                             RawVariables rawVariables, Locale locale) {


Super nit: add new line before Locale

dondonz · 2022-08-08T04:47:26Z

src/main/java/graphql/execution/ValuesResolver.java

-                                                GraphQLType graphQLType,
-                                                Object value) throws NonNullableValueCoercedAsNullException, CoercingParseValueException {
+                                                       GraphQLType graphQLType,
+                                                       Object value, Locale locale) throws NonNullableValueCoercedAsNullException, CoercingParseValueException {


Super nit: add new line before Locale

bbakerman · 2022-08-09T05:30:49Z

src/main/java/graphql/schema/CoercingEnvironment.java

+ *
+ * @param <T> for two
+ */
+public interface CoercingEnvironment<T> {


Feedback - kill it - its not an overhead we want to add in terms of object allocation

…essages in place

…coercing # Conflicts: # src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java

…lass

bbakerman · 2022-08-15T10:33:12Z

src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java

+                CoercedVariables.of(variables),
+                GraphQLContext.getDefault(),
+                Locale.getDefault());
        QueryVisitorFieldEnvironment environment = new QueryVisitorFieldEnvironmentImpl(isTypeNameIntrospectionField,


This entry point come not from a graphql request and hence uses a default graphql context and default locale. Perhaps in some future change these are passed in.

bbakerman · 2022-08-15T10:33:22Z

src/main/java/graphql/analysis/QueryTraverser.java

        this.roots = singletonList(getOperationResult.operationDefinition);
        this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition);
-        this.coercedVariables = ValuesResolver.coerceVariableValues(schema, variableDefinitions, rawVariables);
+        this.coercedVariables = ValuesResolver.coerceVariableValues(schema, variableDefinitions, rawVariables, GraphQLContext.getDefault(), Locale.getDefault());


This entry point come not from a graphql request and hence uses a default graphql context and default locale. Perhaps in some future change these are passed in.

bbakerman · 2022-08-15T10:33:32Z

src/main/java/graphql/execution/ConditionalNodes.java

        Directive foundDirective = NodeUtil.findNodeByName(directives, directiveName);
        if (foundDirective != null) {
-            Map<String, Object> argumentValues = ValuesResolver.getArgumentValues(SkipDirective.getArguments(), foundDirective.getArguments(), CoercedVariables.of(variables));
+            Map<String, Object> argumentValues = ValuesResolver.getArgumentValues(SkipDirective.getArguments(), foundDirective.getArguments(), CoercedVariables.of(variables), GraphQLContext.getDefault(), Locale.getDefault());


This entry point come not from a graphql request and hence uses a default graphql context and default locale. Perhaps in some future change these are passed in.

bbakerman · 2022-08-15T10:33:51Z

src/main/java/graphql/execution/Execution.java

        CoercedVariables coercedVariables;
        try {
-            coercedVariables = ValuesResolver.coerceVariableValues(graphQLSchema, variableDefinitions, inputVariables);
+            coercedVariables = ValuesResolver.coerceVariableValues(graphQLSchema, variableDefinitions, inputVariables, executionInput.getGraphQLContext(), executionInput.getLocale());


context and locale take from ExecutionInput

bbakerman · 2022-08-15T10:34:27Z

src/main/java/graphql/execution/ValuesResolverLegacy.java

+        }
+        return serialized == null;
+    }
+}


Legacy code moved

bbakerman · 2022-08-15T10:39:35Z

src/main/java/graphql/execution/nextgen/FetchedValueAnalyzer.java


    protected Object serializeScalarValue(Object toAnalyze, GraphQLScalarType scalarType) throws CoercingSerializeException {
-        return scalarType.getCoercing().serialize(toAnalyze);
+        return scalarType.getCoercing().serialize(toAnalyze, GraphQLContext.getDefault(), Locale.getDefault());


code will die - here only to compile

bbakerman · 2022-08-15T10:39:59Z

src/main/java/graphql/i18n/I18n.java

     */
    public enum BundleType {
+        Parsing,
+        Scalars,


Our new i18n areas

bbakerman · 2022-08-15T10:40:44Z

src/main/java/graphql/introspection/Introspection.java

-    private static String printDefaultValue(InputValueWithState inputValueWithState, GraphQLInputType type) {
-        return AstPrinter.printAst(ValuesResolver.valueToLiteral(DEFAULT_FIELD_VISIBILITY, inputValueWithState, type));
+    private static String printDefaultValue(InputValueWithState inputValueWithState, GraphQLInputType type, GraphQLContext graphQLContext, Locale locale) {
+        return AstPrinter.printAst(ValuesResolver.valueToLiteral(inputValueWithState, type, graphQLContext, locale));


The method defaults this. Used a better method

…coercing

…coercing # Conflicts: # src/main/java/graphql/execution/nextgen/ExecutionHelper.java # src/main/java/graphql/execution/nextgen/FetchedValueAnalyzer.java # src/main/java/graphql/execution/nextgen/ValueFetcher.java # src/main/java/graphql/schema/idl/FetchSchemaDirectiveWiring.java

dondonz · 2022-08-17T05:25:57Z

src/main/resources/i18n/Scalars.properties

+Int.notInt=Expected a value that can be converted to type 'Int' but it was a ''{0}''
+#
+ID.unexpectedAstType=Expected an AST type of ''IntValue'' or ''StringValue'' but it was a ''{0}''.
+ID.notId=Expected a value that can be converted to type 'ID' but it was a ''{0}''


Nit: could remove the single quotes around 'ID', they won't be rendered

dondonz · 2022-08-17T05:26:10Z

src/main/resources/i18n/Scalars.properties

+ID.unexpectedAstType=Expected an AST type of ''IntValue'' or ''StringValue'' but it was a ''{0}''.
+ID.notId=Expected a value that can be converted to type 'ID' but it was a ''{0}''
+#
+Float.notFloat=Expected a value that can be converted to type 'Float' but it was a ''{0}''


Nit: could remove the single quotes around 'Float', they won't be rendered

dondonz · 2022-08-17T05:26:22Z

src/main/resources/i18n/Scalars.properties

+Float.notFloat=Expected a value that can be converted to type 'Float' but it was a ''{0}''
+Float.unexpectedAstType=Expected an AST type of ''IntValue'' or ''FloatValue'' but it was a ''{0}''.
+#
+Boolean.notBoolean=Expected a value that can be converted to type 'Boolean' but it was a ''{0}''


Nit: could remove the single quotes around 'Boolean', they won't be rendered

sgerke-1L · 2022-08-25T07:17:52Z

src/main/java/graphql/schema/Coercing.java

+     * @param graphQLContext the graphql context in place
+     * @param locale         the locale to use
+     *
+     * @return a parsed value which is never null


Since this method doesn't carry the @NotNull return annotation anymore, should this comment be updated?

Thanks, that's a good catch. I'll fix it up

bbakerman added 5 commits August 5, 2022 13:45

A propose shape change on Coercing

f56f937

deprecation

5919da0

Added parser new signature

dffefc4

Changed ValueResolver so it took Locale

c97b558

Changed calls to coerce to use new environment

5386876

dondonz reviewed Aug 8, 2022

View reviewed changes

bbakerman commented Aug 9, 2022

View reviewed changes

bbakerman added 2 commits August 9, 2022 22:09

Changed calls to coerce to NOT use new environment

62ccc28

Updated tests to use Locale

68b6c2e

bbakerman changed the title ~~A propose shape change on Coercing~~ Adding Locale to Parser, Coercing and hence ValueResolver Aug 10, 2022

bbakerman added 4 commits August 10, 2022 10:30

Formatting of ValuesResolver

39fcdab

Move to graphql context for coercing and also got the parsing error m…

6ccfa79

…essages in place

Merge remote-tracking branch 'origin/master' into propose_changes_to_…

655faad

…coercing # Conflicts: # src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java

Merged in master

d6bfb9c

bbakerman added this to the 20.0 milestone Aug 12, 2022

bbakerman added 5 commits August 14, 2022 20:28

Added Locale to Coercing and everywhere that calls it

89c8574

now using Locale in the standard scalars

da0bd0e

Break out legacy ValuesResolver code

2d74845

Some renaming of methods

68d1070

Refactoring ValueResolver so that conversion methods are in another c…

9d42a8d

…lass

bbakerman commented Aug 15, 2022

View reviewed changes

src/main/java/graphql/execution/ValuesResolverLegacy.java

}

return serialized == null;

}

}

Copy link

Member Author

bbakerman Aug 15, 2022

Choose a reason for hiding this comment

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

Legacy code moved

bbakerman commented Aug 15, 2022

View reviewed changes

src/main/java/graphql/i18n/I18n.java

*/

public enum BundleType {

Parsing,

Scalars,

Copy link

Member Author

bbakerman Aug 15, 2022

Choose a reason for hiding this comment

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

Our new i18n areas

bbakerman commented Aug 15, 2022

View reviewed changes

bbakerman added 3 commits August 16, 2022 07:38

DirectivesResolver should take context and locale from input

c615df1

Merge remote-tracking branch 'origin/master' into propose_changes_to_…

f59454d

…coercing

Reverting Parser changes - they can go into their own PR

f003665

bbakerman changed the title ~~Adding Locale to Parser, Coercing and hence ValueResolver~~ Adding Locale to Coercing and hence ValueResolver Aug 15, 2022

dondonz reviewed Aug 17, 2022

View reviewed changes

dondonz approved these changes Aug 17, 2022

View reviewed changes

bbakerman added 2 commits August 17, 2022 19:58

PR feedback

4408cc8

PR feedback- fixed tests

f207309

This was referenced Aug 19, 2022

Internationalization of graphql-java error messages #1465

Closed

Pass context into scalar coercing #1893

Closed

bbakerman merged commit e7409ac into master Aug 22, 2022

sgerke-1L reviewed Aug 25, 2022

View reviewed changes

sgerke-1L mentioned this pull request Aug 25, 2022

allow Coercing.parseLiteral to return null (remove @NonNull annotation) #2904

Closed

renovate bot mentioned this pull request Nov 14, 2023

Update dependency com.graphql-java:graphql-java to v230521 - autoclosed chains-project/besu#87

Closed

1 task

Comments

Conversation

bbakerman commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Aug 5, 2022 •

edited

Loading