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

Broken code generation for empty enums #1253

Open
cornerman opened this issue Jan 12, 2022 · 11 comments
Open

Broken code generation for empty enums #1253

cornerman opened this issue Jan 12, 2022 · 11 comments
Labels
enhancement New feature or request good first issue Good for newcomers tools Issue related to Caliban tools like code generation or schema comparison

Comments

@cornerman
Copy link
Contributor

We are working with a hasura-generated schema, which somehow contains empty enums like this one:

enum my_empty_enum {}

But when compiling the generated code, we get this compile error:

[error]      type mismatch;
[error]        scala.Unit|caliban.client.ArgEncoder[example.hasura.unauthorized.graphql.client.HasuraUnauthorized.my_empty_enum]
[error]      L523:     implicit val encoder: ArgEncoder[my_empty_enum]    = {}

I am not sure what an empty enum is supposed to mean. It is only empty when accessing hasura unauthorized, but has values when accessing it authorized.

Should this be supported or is that an invalid schema?

@ghostdogpr
Copy link
Owner

See my comment in the other issue, that's an invalid schema. We won't be able to generate a proper query for it.

@cornerman
Copy link
Contributor Author

Thank you! Does it make sense to fail early while parsing the schema? That way you get a proper error message. It is already the case when you try the generate code for enum my_empty_enum.

@ghostdogpr
Copy link
Owner

ghostdogpr commented Jan 15, 2022

Let's catch that case and fail with a nice error message rather than generating code that doesn't compile.

@ghostdogpr ghostdogpr added enhancement New feature or request good first issue Good for newcomers tools Issue related to Caliban tools like code generation or schema comparison labels Jan 15, 2022
@jyoo980
Copy link
Contributor

jyoo980 commented Jan 31, 2022

I can take this issue on. From a preliminary investigation, I can see that there does appear to be some early validation for non-empty enum values in Validator.scala

private[caliban] def validateEnum(t: __Type): IO[ValidationError, Unit] =
t.enumValues(__DeprecatedArgs(Some(true))) match {
case Some(_ :: _) => IO.unit
case _ =>
failValidation(
s"Enum ${t.name.getOrElse("")} doesn't contain any values",
"An Enum type must define one or more unique enum values."
)
}

So I'm wondering why this doesn't appear to be catching the case where there's an empty enum. That said, I'm not super familiar with the system.

It looks like the place to catch it would likely be in SchemaWriter.scala, is that right?

@ghostdogpr
Copy link
Owner

@jyoo980 that would be in ClientWriter.scala

@jyoo980
Copy link
Contributor

jyoo980 commented Feb 1, 2022

Got it, I see this function

def writeEnum(
typedef: EnumTypeDefinition,
extensibleEnums: Boolean
): String = {

A pretty hacky way of catching an empty enum would be to add something like

require(typedef.enumValuesDefinition.nonEmpty, s"Code generation for empty enum '${typedef.name}' failed")

to line 668, but that doesn't seem quite right, do you have any recommendations? It's pretty difficult to see failure cases for this specific object since I think it's predicated on the assumption that most invariants are checked beforehand.

That said, there does seem to be some precedent for this kind of early escape hatch; line 580 throws an exception when codegen is attempted for an object with less than one field:

case Nil => throw new Exception("Invalid GraphQL Schema: an object must have at least one field")

Do you have a preference?

@ghostdogpr
Copy link
Owner

I think it's fine to fail like on line 589 👍
We want to fail the code generation instantly with a useful message rather than generate code that doesn't compile.

@jyoo980
Copy link
Contributor

jyoo980 commented Feb 2, 2022

I'd be happy to add some tests. Do you have any pointers?

Mixing exceptions with functional effects makes thing kinda hairy; I don't think there's a super nice way of testing this as it is right now.

@ghostdogpr
Copy link
Owner

ghostdogpr commented Feb 2, 2022

In ClientWriterSpec, you can wrap the call to ClientWriter.write into a Task so that the exception is caught as a Failure rather than Die. Then with ZIO Test, you can do something like assertM(myEffect.run)(fails(someAssertionAboutTheFailure)). Have a look at check in ValidationSpec for an example.

@jyoo980
Copy link
Contributor

jyoo980 commented Feb 2, 2022

Interesting, I have something like

      testM("enum with no body") {
        val schema =
          """
             enum Origin { }
            """.stripMargin
        assertM(ZIO.effect(gen(schema)).run)(fails(isSubtype[Exception](anything)))
      },

Which yields

 Success(zio.ZIO$FlatMap@468b5697) did not satisfy fails(isSubtype(Exception))

Strange, since the gen(scheme) should be throwing an exception inside ZIO.effect, which should cause it to be a Failure

@ghostdogpr
Copy link
Owner

gen returns a Task, so wrapping it into a Task will execute nothing (you'll have a Task[Task[A]]). You need to wrap ClientWriter.write and flatmap on it so that gen still return a Task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers tools Issue related to Caliban tools like code generation or schema comparison
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants