Schema from named domain object containers changes during execution and exposes internal types

[big-guy]

See tests in: https://github.com/gradle/gradle/compare/eskatos/container/schema

This applies to all named domain object collections, but the examples below use the task container.

Expected Behavior

tasks.create("foo") // foo is a DefaultTask
assert tasks.collectionSchema["foo"] == Task 

tasks.register("bar") // bar is a DefaultTask
assert tasks.collectionSchema["bar"] == Task
tasks.getByName("bar")
assert tasks.collectionSchema["bar"] == Task

Current Behavior

tasks.create("foo") // foo is a DefaultTask
assert tasks.collectionSchema["foo"] == Task  // fails, DefaultTask when created

tasks.register("bar") // bar is a DefaultTask
assert tasks.collectionSchema["bar"] == Task // works
tasks.getByName("bar")
assert tasks.collectionSchema["bar"] == Task // fails, DefaultTask once realized

Context

Kotlin-DSL uses this schema information to generate accessors. Depending on which elements are realized in a container, a different schema can be created for the same collection. This has a performance hit and unnecessarily invalidates their accessors cache.

This also causes problems for users because the accessors may switch between their public and implementation types, depending on if the element was realized or not ; causing e.g. scripts that previously compiled to suddenly fail to compile.

Public type information can come from:

• container element base type
• registered factory type
• registration of new elements with type (create() or register())
• inferred (add(), HasPublicType, firstNonSynthetic ....)

[eskatos]

Note that using HasPublicType is a trap as it requires the instances hence the container elements to be realized. This makes it unsuitable for use in container schema calculation as it may cause schema changes.

[lacasseio]

I assume we want to fix the following TODO for this issue: https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/java/org/gradle/api/internal/DefaultNamedDomainObjectCollection.java#L422-L428

Would change the container to carry the domain object type together with the object or did we discussed a different way of fixing this issue?

[lacasseio]

Strangely, I'm not seeing the behavior shown in this issue:

tasks.create("foo") // foo is a DefaultTask
assert tasks.collectionSchema["foo"] == Task  // fails, DefaultTask when created

tasks.register("bar") // bar is a DefaultTask
assert tasks.collectionSchema["bar"] == Task // fails, DefaultTask is reported
tasks.getByName("bar")
assert tasks.collectionSchema["bar"] == Task // fails, some sort of mock object (the created element)

As for the PolymorphicDomainObjectContainer, I'm seeing all of them as DefaultPerson. All those result use the unit tests for containers.

[lacasseio]

My understanding is we want to 1) always show the same type regardless of the element's realization and 2) show the public type aka:

• tasks.create("foo") -> Task
• tasks.create("foo", SomeType) -> SomeType
• tasks.register("foo") -> Task
• tasks.register("foo", SomeType) -> SomeType
• SomeOtherType t = new SomeOtherType(); tasks.add(t) -> SomeOtherType
• def t = providerOfType(SomeYetOtherType); tasks.addLater(t) -> SomeYetOtherType

Am I understanding the issue properly?

[big-guy]

@lacasseio here's the tests that Paul added: https://github.com/gradle/gradle/blob/7867a09595992cb2c61b30265e0e49dd6f69b6b5/subprojects/core/src/test/groovy/org/gradle/api/internal/DefaultPolymorphicDomainObjectContainerTest.groovy#L342

I think you're right about what we want. TaskContainer might be a bad example since I think we usually want the implementation type.

I think the bigger issue is with the Configuration container where they're getting DefaultConfiguration when they should only see Configuration.

[eskatos]

In practice, on NamedDomainObjectContainers we see:

• DefaultConfiguration instead of Configuration
• DefaultSourceSet instead of SourceSet
• DefaultDistribution instead of Distribution
• etc...

on PolymorphicDomainObjectContainers, we see the implementation type, which is good, except for the "default" type where we get DefaultTask instead of just Task.

[eskatos]

About the schema not changing on element realization. DslObject.getPublicType(some) must not use HasPublicType and should only be used when element instances are given to the container: e.g. using container.add(some).

[eskatos]

Then there are the registered element factories, polymorphic container or not. Those factories are registered with a "public type" and the actual factory that can use another type for the actual element implementation. It would be good for the public type of elements created by such factories exposed by the container schema to be the type used at factory registration time instead of the element implementation type. But, if I understand correctly, fixing this could happen later as the use cases I described two comments above are not impacted.

[big-guy]

Here is a simple fix for some of this: https://github.com/gradle/gradle/pull/6946

The task container behavior is unchanged. You get whatever type is used (and if its not specified, you get a DefaultTask). Since DefaultTask is a public type, this is good enough.

For all other containers, the schema provides only the public type for the container. So for configurations, you get a Configuration instead of a DefaultConfiguration. But for any container that has factories, you'll still get the public type only. This primarily affects the publications, repositories and report containers. There are several others, but they're all software model related.

How close does this get us @eskatos? If I switch the Gradle build to use a local build, can I see the effect in Kotlin DSL?

[eskatos]

@big-guy, I think it's a great first step! It will prevent the leakage of internal types, good enough for now.

If I switch the Gradle build to use a local build, can I see the effect in Kotlin DSL?

Yes, 1.0-rc-11 that is integrated in gradle/gradle master will show you the effect of that change. Try e.g.:

plugins { java }
configurations {
    api {
        // this is Configuration
    }
}
sourceSets {
    main {
        // this is SourceSet
    }
}

[big-guy]

Thanks @eskatos -- I merged the PR to master and updated master to the new nightly.