search

Netflix / dgs-codegen

Apache License 2.0
177 stars 92 forks source link

Generating code from external schemas in JARs - META-INF requirement #668

Open deweyjose opened 3 months ago

deweyjose commented 3 months ago

Hi team,

Curious why there is a requirement for schema files in external jars to exist under the META-INF folder.

This was uncovered in a maven graphql-codegen plugin project here. I'll update our documentation, but wondering why this is a requirement and does it make sense to update the documentation?

srinivasankavitha commented 3 months ago

Usually schema files are added under src/main/resources for graphql projects and hence we scan for the META-INF folder in codegen. Thanks for pointing out the documentation gap, we need to fix that.

srinivasankavitha commented 3 months ago

Fixed the docs here: https://github.com/Netflix/dgs/commit/a518e6d1af69d591fba88bc9d3b06f5c49862290

deweyjose commented 3 months ago

@srinivasankavitha

Usually schema files are added under src/main/resources for graphql projects and hence we scan for the META-INF folder in codegen. Thanks for pointing out the documentation gap, we need to fix that.

Even though the graphqls files are typically under src/main/resources they do not necessarily get packaged in the jar under META-INF. This requirement seems odd to me. All of the .graphql files in the jars I produce are packaged outside of META-INF.

Even Apollo's Federation graphqls files are not packaged under META-INF.

image

Why does the code need to enforce the META-INF prefix vs scanning the entire Jar?

deweyjose commented 3 months ago

It looks like DGS decided to package error related schema in graphql-error-types under src/main/resources/META-INF/schema which is why it's packaged that way, but not all teams want to structure their codebases this way.

image
srinivasankavitha commented 3 months ago

Ok, I see. At the time of adding this feature, our primary use case was to pick up schemas under META-INF and from other modules in a given project that has schemas under META-INF, so the current implementation addresses that requirement. But for other external dependencies it would not work as you just pointed out. We could enhance this implementation to scan for .graphqls files more generally as well I suppose.

deweyjose commented 3 months ago

@srinivasankavitha thanks for taking a second look and reconsidering!