Skip to content
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

feat!: Update to Groovy 4 #13532

Merged
merged 10 commits into from
Jul 11, 2024
Merged

feat!: Update to Groovy 4 #13532

merged 10 commits into from
Jul 11, 2024

Conversation

matrei
Copy link
Contributor

@matrei matrei commented Jul 2, 2024

No description provided.

The GROOVY_4_0_X branch has split the Develocity config
into multiple files. This commit is meant to accommodate for that.
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the failing test from @ignore to PendingFeature instead?

One JUnit test is replaced with Spock to be able to use `@PendingFeature`
as I could not find an equivalent annotation in JUnit.
This test passes now, not sure why it failed before
@matrei matrei requested a review from sdelamo July 4, 2024 13:44
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrei Thanks for doing this PR. Moreover, thanks for doing such a granular PR. It makes review really easy.

}
}

@Entity
//@Entity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test won't compile with the @Entity annotation present when extending another domain class.

We could also do this:

// With Groovy 4, it is currently not possible to extend domain classes: https://issues.apache.org/jira/browse/GROOVY-5106
@Entity
class StringPropertyValue /*extends AbstractCustomPropertyValue*/ {

    String stringValue

    static constraints = {
        stringValue (nullable: true)
    }

    StringPropertyValue (String value) {
        this.stringValue = value
        this.valid = true
    }
}

@@ -67,6 +68,7 @@ class DomainConstraintGettersSpec extends Specification implements DataTest {

// DOMAIN WITH SUPER CLASS

@PendingFeature(reason = 'With Groovy 4, it is currently not possible to extend domain classes: https://issues.apache.org/jira/browse/GROOVY-5106')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://issues.apache.org/jira/browse/GROOVY-5106 seems to be fixed. are we not using a groovy version which does not contain the patch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulk-asert do you know about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the fix to that issue that causes the problem with extending domain classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the following classes in Java:

class Parent implements Iterator<CharSequence> { ... }
class Foo extends Parent implements Iterator<String> { ... }

Java will tell you to remove one of the Iterator interface clauses:

'java.util.Iterator' cannot be inherited with different type arguments: 'java.lang.CharSequence' and 'java.lang.String'. 

Groovy 3 allowed that to co-exist since after type erasure, it folds down onto one interface and the one you want was kept. But that was a kludge and information was lost, so Groovy 4 does the same as Java here.

In this case we have (note duplicated GormEntity and Entity with different types):

class AbstractCustomPropertyValue implements GroovyObject, GormEntity<AbstractCustomPropertyValue>, GormEntityDirtyCheckable, WebDataBinding, DomainClass, grails.gorm.Entity<AbstractCustomPropertyValue>, DirtyCheckable.Trait.FieldHelper, GormValidateable.Trait.FieldHelper { ... }
class StringPropertyValue extends AbstractCustomPropertyValue implements GormEntity<StringPropertyValue>, WebDataBinding, DomainClass, grails.gorm.Entity<StringPropertyValue>, DirtyCheckable.Trait.FieldHelper, GormValidateable.Trait.FieldHelper { ... }

If supporting inheritance of entities is desirable, the @Entity transform would need to be made smarter. There might be a few different ways to make it work. Perhaps only abstract classes could be extended and they might not need the original interface added. Perhaps non-final classes could use a generic type like ? extends AbstractCustomProperty (in this case).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My example was for the previous test but the discussion still applies. For the previous test, I think you can also remove @Entity from the abstract parent class (and remove it from mockDomains too) and the test still passes prior to this PR at least.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrei can we update the test and start a breaking changes section in the docs telling the user about this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdelamo Yes, we could. Are you aware of any use cases where extended domain classes are also used concretely? That kind of usage would obviously not work anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could try running https://github.com/grails/grails-functional-tests project to see if anything is broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could try running https://github.com/grails/grails-functional-tests project to see if anything is broken.

Hi @puneetbehl, is it possible to run the grails-functional-tests project on the this branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you would need to make certain changes to the build.gradle file of the project.

@matrei matrei requested a review from sdelamo July 9, 2024 14:08
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to merge this into 7.0.x branch. We can work on the @PendingFeature tests in separate PRs. Thanks for the work.

@sdelamo sdelamo merged commit 6c1bc8c into grails:7.0.x Jul 11, 2024
11 checks passed
@matrei matrei deleted the matrei/groovy4 branch September 30, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants