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

Incorrect arguments ExecutionParams in graphql should throw proper GraphQL errors #2507

Closed
schrockn opened this issue May 25, 2020 · 1 comment
Assignees
Labels
type: good first issue Good for newcomers

Comments

@schrockn
Copy link
Member

schrockn commented May 25, 2020

In create_execution_params:

def create_execution_params(graphene_info, graphql_execution_params):

    preset_name = graphql_execution_params.get('preset')
    selector = PipelineSelector.from_graphql_input(
        graphene_info.context, graphql_execution_params['selector']
    )
    if preset_name:
        check.invariant(
            not graphql_execution_params.get('environmentConfigData'),
            'Invalid ExecutionParams. Cannot define environment_dict when using preset',
        )
        check.invariant(
            not graphql_execution_params.get('mode'),
            'Invalid ExecutionParams. Cannot define mode when using preset',
        )

        check.invariant(
            not selector.solid_subset,
            'Invalid ExecutionParams. Cannot define selector.solid_subset when using preset',
        )

We do invariant checks that fail when we pass in properly formatted graphql. We should be throwing proper GraphQL errors. "check" errors are the moral equivalent of something akin to a typecheck failure, where as the errors above are more "business domain" errors. The computation has completed normally and has failed in an expected manner.

The fact that this is so odd is exposed here:


    def test_basic_start_pipeline_execution_with_preset_failure(self, graphql_context):
        with pytest.raises(check.CheckError):
            execute_dagster_graphql(
                graphql_context,
                START_PIPELINE_EXECUTION_QUERY,
                variables={
                    'executionParams': {
                        'selector': {'name': 'csv_hello_world', 'solidSubset': 'sum_sq_solid'},
                        'preset': 'test_inline',
                    }
                },
            )
schrockn added a commit that referenced this issue May 26, 2020
…cking codepath

Summary:
Continuing to move code over to the hijacking codepath. Found some weirdness
in that we are using check to commmunicate graphql domain errors.

Tracking here #2507

Depends on D3087

Test Plan: BK

Reviewers: alangenfeld, max

Reviewed By: alangenfeld

Differential Revision: https://dagster.phacility.com/D3088
@natekupp natekupp added this to the 0.8.0 milestone Jun 1, 2020
@schrockn schrockn removed this from the 0.8.0 milestone Jun 6, 2020
@schrockn schrockn added the type: good first issue Good for newcomers label Jun 6, 2020
@helloworld
Copy link
Contributor

helloworld commented Jun 18, 2020

For a bit more additional context, we want to introduce a new error type, similar to the PresetNotFoundError that's right after the code snippet defined above.

  if not external_pipeline.has_preset(preset_name):
            raise UserFacingGraphQLError(
                graphene_info.schema.type_named('PresetNotFoundError')(
                    preset=preset_name, selector=selector
                )
            )

Instead of doing check.invariant, we'll want to raise a new UserFacingGraphQLError with a type describing this class of error where both a preset and one of environmentConfigData, mode or solidSubset is defined.

ajnadel added a commit that referenced this issue Jun 19, 2020
Summary:
When checking for conflicts between presets and execution params, return a GraphQL error type (ConflictingExecutionParamsError) instead of raising invariant errors. Ensure that these errors are caught and returned through gql result rather than simply raised.

See [[ #2507 | #2507 ]]

Test Plan: Updated unit tests in `python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_execute_pipeline.py` to match new error format.

Reviewers: sashank

Reviewed By: sashank

Differential Revision: https://dagster.phacility.com/D3559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants