Remove redundant call to validate

[shukryzablah]

The call to validate in the django view is redundant with the validation call in graphql-core. Related to #1198

[erikwrede]

Thanks for opening the PR!

While this PR brings a big reduction in overhead, we still have some unnecessary overhead left due to the call of parse. On discord, I suggested moving the execute logic into the function as well to remove that overhead. It would simply involve replacing the schema.execute call with a call of graphene's internal execute including the parsed AST.

For reference, this is how GQL-Core handles execution of a query string:

  #excerpt of graphql core's graphql_impl
    """Execute a query, return asynchronously only if necessary."""
    # Validate Schema
    schema_validation_errors = validate_schema(schema)
    if schema_validation_errors:
        return ExecutionResult(data=None, errors=schema_validation_errors)

    # Parse
    try:
        document = parse(source)
    except GraphQLError as error:
        return ExecutionResult(data=None, errors=[error])

    # Validate
    from .validation import validate

    validation_errors = validate(schema, document)
    if validation_errors:
        return ExecutionResult(data=None, errors=validation_errors)

    # Execute, just move this call into graphene-django instead of graphene
    return execute(
        schema, # replace with graphene_schema.graphql_schema
        document,
        root_value,
        context_value,
        variable_values,
        operation_name,
        field_resolver,
        type_resolver,
        None,
        middleware,
        execution_context_class,
        is_awaitable,
    )

Since 2/3 of this logic is already duplicated in the present graphene-django implementation, it might be worthwhile to move the rest there as well. Personally, I'm fine with both and believe this could also be merged as is while we evaluate my suggested addition. - cc @firaskafri @tcleonard

[firaskafri]

Thanks for opening the PR!

While this PR brings a big reduction in overhead, we still have some unnecessary overhead left due to the call of parse. On discord, I suggested moving the execute logic into the function as well to remove that overhead. It would simply involve replacing the schema.execute call with a call of graphene's internal execute including the parsed AST.

For reference, this is how GQL-Core handles execution of a query string:

  #excerpt of graphql core's graphql_impl
    """Execute a query, return asynchronously only if necessary."""
    # Validate Schema
    schema_validation_errors = validate_schema(schema)
    if schema_validation_errors:
        return ExecutionResult(data=None, errors=schema_validation_errors)

    # Parse
    try:
        document = parse(source)
    except GraphQLError as error:
        return ExecutionResult(data=None, errors=[error])

    # Validate
    from .validation import validate

    validation_errors = validate(schema, document)
    if validation_errors:
        return ExecutionResult(data=None, errors=validation_errors)

    # Execute, just move this call into graphene-django instead of graphene
    return execute(
        schema, # replace with graphene_schema.graphql_schema
        document,
        root_value,
        context_value,
        variable_values,
        operation_name,
        field_resolver,
        type_resolver,
        None,
        middleware,
        execution_context_class,
        is_awaitable,
    )

Since 2/3 of this logic is already duplicated in the present graphene-django implementation, it might be worthwhile to move the rest there as well. Personally, I'm fine with both and believe this could also be merged as is while we evaluate my suggested addition. - cc @firaskafri @tcleonard

Thank you for pin pointing this! Will look into it with @mohsam97

[firaskafri]

@shukryzablah Hello! Can you please the failing test? its just trim trailing whitespace.................................................Failed

[firaskafri]

Thanks for opening the PR! While this PR brings a big reduction in overhead, we still have some unnecessary overhead left due to the call of parse. On discord, I suggested moving the execute logic into the function as well to remove that overhead. It would simply involve replacing the schema.execute call with a call of graphene's internal execute including the parsed AST. For reference, this is how GQL-Core handles execution of a query string:

  #excerpt of graphql core's graphql_impl
    """Execute a query, return asynchronously only if necessary."""
    # Validate Schema
    schema_validation_errors = validate_schema(schema)
    if schema_validation_errors:
        return ExecutionResult(data=None, errors=schema_validation_errors)

    # Parse
    try:
        document = parse(source)
    except GraphQLError as error:
        return ExecutionResult(data=None, errors=[error])

    # Validate
    from .validation import validate

    validation_errors = validate(schema, document)
    if validation_errors:
        return ExecutionResult(data=None, errors=validation_errors)

    # Execute, just move this call into graphene-django instead of graphene
    return execute(
        schema, # replace with graphene_schema.graphql_schema
        document,
        root_value,
        context_value,
        variable_values,
        operation_name,
        field_resolver,
        type_resolver,
        None,
        middleware,
        execution_context_class,
        is_awaitable,
    )

Since 2/3 of this logic is already duplicated in the present graphene-django implementation, it might be worthwhile to move the rest there as well. Personally, I'm fine with both and believe this could also be merged as is while we evaluate my suggested addition. - cc @firaskafri @tcleonard

Thank you for pin pointing this! Will look into it with @mohsam97

https://github.com/graphql-python/graphene-django/pull/1439