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

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

Closed
big-guy opened this Issue Sep 20, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@big-guy
Member

big-guy commented Sep 20, 2018

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 ....)

@big-guy big-guy changed the title from Schema from domain object containers changes during execution and exposes internal types to Schema from named domain object containers changes during execution and exposes internal types Sep 20, 2018

@eskatos

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Sep 24, 2018

Member

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.

Member

eskatos commented Sep 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lacasseio

lacasseio Sep 28, 2018

Member

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?

Member

lacasseio commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lacasseio

lacasseio Sep 28, 2018

Member

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.

Member

lacasseio commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lacasseio

lacasseio Sep 28, 2018

Member

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?

Member

lacasseio commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@big-guy

big-guy Sep 28, 2018

Member

@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.

Member

big-guy commented Sep 28, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Sep 28, 2018

Member

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.

Member

eskatos commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Sep 28, 2018

Member

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).

Member

eskatos commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Sep 28, 2018

Member

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.

Member

eskatos commented Sep 28, 2018

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 big-guy assigned big-guy and unassigned lacasseio Sep 29, 2018

@big-guy

This comment has been minimized.

Show comment
Hide comment
@big-guy

big-guy Sep 29, 2018

Member

Here is a simple fix for some of this: gradle/gradle#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?

Member

big-guy commented Sep 29, 2018

Here is a simple fix for some of this: gradle/gradle#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

This comment has been minimized.

Show comment
Hide comment
@eskatos

eskatos Sep 29, 2018

Member

@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
    }
}
Member

eskatos commented Sep 29, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@big-guy

big-guy Sep 30, 2018

Member

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

Member

big-guy commented Sep 30, 2018

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

@big-guy big-guy closed this Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment