Custom library-defined traits don't work with the codegen plugin

[schmeedy]

We might be doing something wrong, but it seems that custom library-defined traits are not properly supported by the codegen plugin.

Our setup:

• we have a library which defines some custom traits and registers the TraitService SPIs for them as well as some validators
• a service project is using the smithy4s codegen plugin & it depends on the above library as well as some JARs with smithy files

The issue: When the model is loaded in the service project, it can't see the TraitService SPIs for our custom traits and it falls back to the dynamic model for those. Validators are picked-up fine, but they can't see the traits in the model since they rely on the runtime classes and won't work correctly with the dynamic model. Thus, we get validation errors everywhere.

The cause: The ModelLoader is only passing the classloader that can see external dependencies to the discoverModels call of the assembler and not to the factory method for the assembler itself:

Model
  .assembler() // this is using current classloader (which can only see the plugin's classpath)
  .discoverModels(upstreamClassLoader)

https://github.com/disneystreaming/smithy4s/blob/5ce1106e363bf1bec78e196791cbb26da6d803de/modules/codegen/src/smithy4s/codegen/ModelLoader.scala#L58 This way, it can't see any TraitService SPIs contributed by external dependencies, but it can still discover & load the smithy files contained in them.

Our attempts to fix this: If we just pass the upstreamClassloader to the Model.assembler call, it loads the TraitService interface again from the smithy-model jar through this classloader and you end up in a situation where the SPI lookup fails with "TraitService is not a TraitService" type of error. We ran into similar kinds of issues when adding the current classloader as a parent of upstreamClassLoader and attempting to filter the external classpath.

It works if we abandon the staged model loading approach altogether and load everything from a unified classpath:

val validatorClassLoader = maybeDeps match {
  case Some(deps) => new URLClassLoader(deps, currentClassLoader)
  case None       => currentClassLoader
}

val modelAssembler =
  Model
    .assembler(validatorClassLoader)
    .discoverModels(validatorClassLoader) // discovering models from both internal & external classpath in a single pass

But that would surely break other things.

[Baccata]

@schmeedy thanks for reporting, in particular in such a well described fashion

We've found that relying on discoverModels with the current class loader can be dangerous : we internally have some other plugins that rely on the smithy toolset, and in projects that use both smithy4s and those other plugins, the model discovery was capturing more than it should have. It could be fixed by having the SBT plugin shell-out to the smithy4s-cli (fetching it via coursier) instead of using smithy4s-codegen directly. That would certainly help the "surely break other things" bit, but would add to the maintenance burden by not being able to rely on type-checking when the CodegenArgs change for whatever reason.

I think using addImport(url: java.net.URL) for the jar dependencies, instead of discoverModels, may help. In theory it'd let us drop the staged-model loading approach, and would be more precise than the wide-raking "discoverModels" (which may load everything on the classpath of your build). discoverModels would only be enabled when encountering the corresponding flag from the CodegenArgs.

If it works for you, open a PR. We'll review carefully and will try it internally, see if we encounter any issue.

[kubukoz]

Update: still happening in 0.18.0

Having looked at the project @schmeedy was referring to in the state at the time this was reported, I think I found the particular corner case that reproduces this.

Consider this spec:

namespace com.kubukoz

use alloy#simpleRestJson

@trait(selector: "service")
string apiVersion

@simpleRestJson
@apiVersion("v1")
service FooService {

}

If I define a trait service + validator... let's consider the validator doing something like this

1. Find all shapes with @simpleRestJson, for each of them do the following:
2. Ensure shape has @apiVersion
3. otherwise ERROR

then the validator doesn't see the real trait, but a dynamic default. At least that's what I'm seeing.

I'm aware that it's probably not a good pattern (adding behavior to a shape I don't own, @simpleRestJson) but that's long gone from the project and at this point I'm just reporting details in case we want to fix this.

Reproduction:

1. Clone https://github.com/kubukoz/demos/tree/smithy4s-validation-scope
2. ./mill demo.publishLocal
3. cs launch com.disneystreaming.smithy4s:smithy4s-codegen-cli_2.13:0.17.0 -- generate --dependencies com.kubukoz:demo:0.0.1-SNAPSHOT

The output contains this:

software.amazon.smithy.model.validation.ValidatedResultException: Result contained ERROR severity validation events: 
[ERROR] com.kubukoz#FooService: Missing @apiVersion trait on service com.kubukoz#FooService. Found traits:

=======================
alloy#simpleRestJson: alloy.SimpleRestJsonTrait
com.kubukoz#apiVersion: software.amazon.smithy.model.traits.DynamicTrait
=======================
 | ApiVersioning jar:file:/Users/kubukoz/.ivy2/local/com.kubukoz/demo/0.0.1-SNAPSHOT/jars/demo.jar!/META-INF/smithy/demo.smithy:10:1

[kubukoz]

I have a vague feeling that this problem may be similar to what was fixed here: https://github.com/awslabs/smithy-language-server/pull/84