Add support for interface implementing interface

[filosganga]

This is an initial support for #567 and supersede #597 that is based on very old base

The rendering has been a bit challenging and I took a non-standard path there

[yanns]

Very cool to see progress here!

[filosganga]

@yanns This is pretty much done but there are a couple of missing validations that I don't know how to handle:

All the implementing fields must be declare
When a type or an interface implement another interface, all the implementing fields must be declared. This is not currently validated even without interfaces implementing interfaces, so can it be addressed in a follow-up PR?

The implementation hierarchy MUST not include loops
I have managed to check for interfaces implementing themselves, however is very complex to spot issues like this:

interface Foo implement Baz {
  foo: String
}

interface Bar implement Foo{
  bar: String
  foo: String
}

interface Baz implement Bar & Foo {
  baz: String
  bar: String
  foo: String
}

Because the materializer/builder goes in SO when trying to resolve the types. Is there a way to validate the ast before running the materializer/builder?

Cheers

[yanns]

All the implementing fields must be declare When a type or an interface implement another interface, all the implementing fields must be declared. This is not currently validated even without interfaces implementing interfaces, so can it be addressed in a follow-up PR?

Yes

The implementation hierarchy MUST not include loops I have managed to check for interfaces implementing themselves, however is very complex to spot issues like this:

interface Foo implement Baz {
  foo: String
}

interface Bar implement Foo{
  bar: String
  foo: String
}

interface Baz implement Bar & Foo {
  baz: String
  bar: String
  foo: String
}

Because the materializer/builder goes in SO when trying to resolve the types. Is there a way to validate the ast before running the materializer/builder?

I'm not sure. I'm thinking of https://github.com/sangria-graphql/sangria/pull/966/files where some validation is done on the AST directly. Does this help?

[filosganga]

All the implementing fields must be declare When a type or an interface implement another interface, all the implementing fields must be declared. This is not currently validated even without interfaces implementing interfaces, so can it be addressed in a follow-up PR?

Yes

The implementation hierarchy MUST not include loops I have managed to check for interfaces implementing themselves, however is very complex to spot issues like this:

interface Foo implement Baz {
  foo: String
}

interface Bar implement Foo{
  bar: String
  foo: String
}

interface Baz implement Bar & Foo {
  baz: String
  bar: String
  foo: String
}

Because the materializer/builder goes in SO when trying to resolve the types. Is there a way to validate the ast before running the materializer/builder?

I'm not sure. I'm thinking of https://github.com/sangria-graphql/sangria/pull/966/files were some validation is done on the AST directly. Does this help?

Yep, I think so.

Also, thank you to have allowed the workflow to run, I need to fix a few things :)

[yanns]

[error] (sangria-core / Compile / scalafmtCheck) scalafmt: 1 files must be formatted (/home/runner/work/sangria/sangria/modules/core)

[filosganga]

@yanns This is ready to review. However, it is impossible to retain binary compatibility because Sangria uses case classes with default values. I have added the mima filters but I think a major version bump may be needed

All the implementing fields must be declare When a type or an interface implement another interface, all the implementing fields must be declared. This is not currently validated even without interfaces implementing interfaces, so can it be addressed in a follow-up PR?

Yes
Actually, I saw that in (

sangria/modules/core/src/main/scala/sangria/schema/SchemaValidationRule.scala

Lines 125 to 127 in 7970fa7

	// we allow object type to inherit fields from the interfaces
	// without explicitly defining them, but only when it is not
	// defined though SDL.

) Sangria allows to avoid defining all the inherited fields.

I have implemented cycle detection in the AST before being materialised

[yanns]

Really cool, I'll have a look at this.
Yes, I think I'll make a major version bump.

[filosganga]

If you do a major version bump, I can remove the apply methods that meant to keep source compatibility or limit the changes.

Thank you for your time

[yanns]

Beautiful PR, with lot of tests! 💯

[yanns]

💯 for taking care of "source compatibility".
I've checked that this apply is only used in tests.

[yanns]

If you do a major version bump, I can remove the apply methods that meant to keep source compatibility or limit the changes.

Good point.
We should still try not to break user code if possible. It's sometimes feasible with default values. But sometimes we need those new apply methods.

[yanns]

For reference, this is in the spec since https://github.com/graphql/graphql-spec/blob/main/changelogs/October2021.md

[filosganga]

Good point. We should still try not to break user code if possible. It's sometimes feasible with default values. But sometimes we need those new apply methods.

Fair point

[yanns]

released in https://github.com/sangria-graphql/sangria/releases/tag/v4.0.0-RC1