Make empty string invalid for IDType

[BjRo]

Hey @OlegIlyenko,

we stumbled about this one in our app. Right now a query such as

query demo {
   car(id: "    ") {
      doors 
  }
}

reaches the resolvers. Is that intended? I would argue that ID should never be an empty string.

I quickly glanced over what the GraphQL spec is telling about blank strings for ID but couldn't find anything.

This PR patches IDType so that it doesn't accept blank strings. What do you think?

[yellowbrickc]

I think this behavior breaks the least astonishment rule among other things. I agree with you, it should not be allowed.

[OlegIlyenko]

Thanks for pointing this out, I think it is a valid concern! I will also look in the spec and ask around about proposed adjustments to the ID type (also will check other implementations).

Meanwhile, maybe you can try to use the scalar alias to validate such bad inputs:

case object EmptyIdViolation extends BaseViolation("ID cannot be an empty string.")

implicit val MyIdType = ScalarAlias[String, String](
  IDType, identity, id ⇒ 
    if (id.trim.isEmpty) Letf(EmptyIdViolation)
    else Right(id))

I believe that you can achieve the same with MiddlewareFromScalar as well.

[OlegIlyenko]

FYI, I also created an issue in the spec repo: graphql/graphql-spec#404

[BjRo]

Unfortunately I didn't have time to work on this last week. But I got the chance to check this morning.

I've checked the middleware approach. With this I'm definitely able to block empty ids".

There's one thing I noticed though: It behaves differently in terms of error handling (compared to errors from coerceInput).

• coerceInput violations ultimately raise an sangria.execution.ValidationError during the execution (execution stops)
• middleware based errors from fromScalar end up in onViolation of the exception handler (and execution continues)

The scalar alias seems to be a bit more involved. If I understand correctly it would mean replacing all references in the Scala code from IDType to our alias. And then we would also need to teach our SDL builder to use our alias type instead of the built-in one. Is that correct?

[OlegIlyenko]

Yes, you are right. Middleware does not participate in the validation process. This would be the difference.

For the scalar alias, you indeed would need to update your builder. Recently the necessary hooks were added. I believe you would need to override:

• buildArgumentType
• buildInputFieldType
• buildFieldType

and return the appropriate alias for an ID scalar type. Here is an example of how buildArgumentType might look like:

def replaceInputType(tpe: ast.Type, namedType: InputType[_], optional: Boolean = true): InputType[_] =
  tpe match {
    case ast.ListType(ofType, _) if optional ⇒ OptionInputType(ListInputType(replaceInputType(ofType, namedType, true)))
    case ast.ListType(ofType, _) ⇒ ListInputType(replaceInputType(ofType, namedType, true))
    case ast.NotNullType(ofType, _) ⇒ replaceInputType(ofType, namedType, false)
    case ast.NamedType(name, _) ⇒ namedType
  }

override def buildArgumentType(
    origin: MatOrigin,
    typeDefinition: TypeSystemDefinition,
    fieldDefinition: Option[FieldDefinition],
    definition: InputValueDefinition,
    defaultValue: Option[Tuple2[_, ToInput[_, _]]],
    mat: AstSchemaMaterializer[Unit]): InputType[Any] =
  if (definition.valueType.namedType.name == "ID") replaceInputType(definition.valueType, MyIdType)
  else super.buildArgumentType(origin, typeDefinition, fieldDefinition, definition, defaultValue, mat)

I think I can also add some helpers in future to make it a bit less verbose.

[BjRo]

@OlegIlyenko We decided to go with the Scalar middleware for now.

Looking at the alternatives, the amount of changes they would introduce for the codebase, we made the tradeoff that we accepted the difference in behaviour.

We also experimented with adding a new validation rules for this, but since this would deal with the whole recursion over the type system https://github.com/sangria-graphql/sangria/blob/master/src/main/scala/sangria/validation/QueryValidator.scala#L177 we stopped there because we felt a bit uneasy with copying and tweaking that.

Maybe there's room for a version of isValidLiteralValue that allows passing in the leaf checks via (partial) function?

[OlegIlyenko]

Maybe there's room for a version of isValidLiteralValue that allows passing in the leaf checks via (partial) function?

I think there is a better approach already available. Since we are talking about the query validation, you can just implement a simple validation that will do the validation:

class NonEmptyIdSting extends ValidationRule {
  override def visitor(ctx: ValidationContext) = new AstValidatingVisitor {
    override val onEnter: ValidationVisit = {
      case str: ast.StringValue if str.value.trim.isEmpty ⇒
        ctx.typeInfo.inputType match {
          case Some(inputType) if inputType.namedType.name == IDType.name ⇒
            Left(Vector(EmptyIdViolation(ctx.sourceMapper, str.position.toList)))
          case _ ⇒
            AstVisitorCommand.RightContinue
        }
    }
  }
}

case class EmptyIdViolation(sourceMapper: Option[SourceMapper], positions: List[Position]) extends AstNodeViolation {
  lazy val simpleErrorMessage = s"ID is empty."
}

this validation will find all instances of an empty IDs and ensure that they are non-empty. You need to consider that this covers only the values that are provided directly in the query. Values also can be provided via variables. To cover these cases would need the middleware, custom scalar/scalar alias or do the validation directly in the resolver.

[OlegIlyenko]

@yellowbrickc @BjRo I would appreciate if you would check this discussion and share your thoughts if you have a strong opinion on the subject matter graphql/graphql-spec#404 (comment)

So at the moment my personal preference would be to avoid changes to the ID type itself and focus on better API for introducing such validations (with things like scalar aliases, validation rules, etc.)

[BjRo]

@OlegIlyenko I'll try the validation rule. I think that would work good enough for us as-well (and be better than the middleware). In general I have a hard time to find use cases for only whitespace IDs, but I understand that you want to be as open as possible to not close doors for some people with such requirements

[BjRo]

@OlegIlyenko Very cool. Your validation rule suggestion works like a charm. That results in the behaviour we wanted to have at a good code ration 🎉

Feel free to close this and thank you again for your help!

[OlegIlyenko]

Great, glad that it worked for you! I also don't consider empty strings or whitespace-only ID to be a good approach (unless people are using the Whitespace to encode numbers :)). I'm just thinking about old legacy systems out there that might have a special interpretation for an empty string IDs. If something like this was introduced at some point in the past, it might be extremely hard to get rid of it later on, especially within bigger enterprise systems.

That said, I would like to stay open to the idea of introduction of such constraint on the ID type. For example, if it will become a stumbling point for people who are starting with GraphQL, then i think it might be a better choice to sacrifice some of the flexibility in favour of more safer defaults. I will close this PR for the moment being, but would love to collect more feedback on the subject matter and revisit this issue at some point in future.