Flaky error handling with MaskErrors

[Speedy1991]

The ValidationCache is writing any errors in the execution_context.errors

    def on_validate(self) -> Iterator[None]:
        execution_context = self.execution_context

        errors = self.cached_validate_document(
            execution_context.schema._schema,
            execution_context.graphql_document,
            execution_context.validation_rules,
        )
        execution_context.errors = errors
        yield

But MaskErrors is checking for self.execution_context.result.errors

    def on_operation(self) -> Iterator[None]:
        yield
        result = self.execution_context.result
        if result and result.errors:

Maybe this bug is in other extensions too?

[Speedy1991]

It would be also nice to have a helper function on SchemaExtension to easily add errors. I saw the ValidatioRule has an report_error, something simmelar would be also nice in SchemaExtension.

My current workaround is a helper function:

def _inject_exception(execution_context: ExecutionContext, error: Exception):
    if not execution_context.result:
        execution_context.result = ExecutionResult()
    if execution_context.result.errors is None:
        execution_context.result.errors = list()
    execution_context.result.errors.append(GraphQLError(str(error), original_error=error))

[Speedy1991]

Just looked into this again. I'm not sure if I should fix these two places:

• https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/extensions/validation_cache.py#L46
• https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/schema/schema.py#L751

Or just make MaskErrors more flexible (https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/extensions/mask_errors.py#L38) and check for context.errors and context.result.errors

I would prefer the first option, but this could brick some custom MaskError implementations.

Maybe it would be more graceful, to show a deprecation warning if there are any errors in the context.errors for some time and drop the support for context.errors later?

[Speedy1991]

To continue my research, it looks like ExecutionContext.errors is some deprecated/legacy code? As far I can see, the tests are only covering result.errors - so maybe one solution would be to make a breaking change and just drop the errors attribute from ExecutionContext.errors?

[Speedy1991]

After talking to @patrick91 and @DoctorJohn, it makes more sense to divide the errors into PreExecutionErrors and Errors (maybe?)

[ericdevries]

I found it weird that there are two kinds of execution results and apparently both are passed to this method. But in either case it has the same properties. Do you know why it is using that other property?

[DoctorJohn]

After talking to @patrick91 and @DoctorJohn, it makes more sense to divide the errors into PreExecutionErrors and Errors (maybe?)

We cleared up some confusion around this in #3947. The ExecutionContext.errors property was renamed to ExecutionContext.pre_execution_errors.

Now ExecutionContext.pre_execution_errors are errors that happened before an operation is executed (e.g., validation errors), and ExecutionContext.result.errors are errors that happened during the execution of the operation.

It appears that the MarkErrors extension needs to be updated to handle both fields correctly (i.e., masking the values of both fields).

[dextermb]

It appears that the MarkErrors extension needs to be updated to handle both fields correctly (i.e., masking the values of both fields).

Is there someone already working on this change or are you looking for new contributors?

[patrick91]

@dextermb happy to take a PR for this 😊

[dextermb]

@dextermb happy to take a PR for this 😊

@patrick91 I've created an initial pull request, currently draft, I've got to do some testing but would be good to get some thoughts from repo-members.