Skip to content

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Aug 29, 2025

Currently RestfulServiceController will not work for situations where you have domain objects with the same name.

This scenario is more likely for complex websites that use plugins with domain objects.

For instance:

grails-app/domain/org/website/User.groovy
grails-app/domain/org/website/plugin/User.groovy
package org.website

import org.springframework.security.provisioning.UserDetailsManager

@Scaffold(GormService<User>)
class UserService implements UserDetailsManager {
    // implementation 
}
package org.website.plugin

@Scaffold(GormService<User>)
class MyPluginUserService  {
    //  implementation 
}

This introduces a fix that will resolve the appropriate service class for the respective GormService

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

We need to add tests for this to merge it.

@codeconsole
Copy link
Contributor Author

@jdaugherty the scaffolding plugin doesn't have any tests? Do you know of any?

This is a fix. If there are no existing tests, it is better to merge than to leave broken, no?

@jdaugherty
Copy link
Contributor

@codeconsole We should add a functional test project. DomainServiceLocator appears more complex than it should be. How do we know an existing workflow didn't break without a test?

@codeconsole
Copy link
Contributor Author

codeconsole commented Sep 5, 2025

@jdaugherty

DomainServiceLocator appears more complex than it should be.

I've simplified it to its original intent. It now just only uses services of type GormService

How do we know an existing workflow didn't break without a test?

RestfulServiceController is a new class that is just an example of the Service Layer pattern. There are no existing workflows and it would be silly to just add 1 test to the entire plugin that only tests this.

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

I've added this to Wednesday's developer meeting to see where people stand on this. I tried to point out in the review some possible problem points. I'm not 100% sure if it would cause compilation issues - but if it does we have to agree to do another RC.

@jamesfredley
Copy link
Contributor

For the scaffolding functional tests, the following existing test projects are a good place to copy as a quick starting point. These all use Geb w/ TestContainers.

https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/geb
https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/gsp-layout
https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/app1

@codeconsole
Copy link
Contributor Author

For the scaffolding functional tests, the following existing test projects are a good place to copy as a quick starting point. These all use Geb w/ TestContainers.

https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/geb https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/gsp-layout https://github.com/apache/grails-core/tree/7.0.x/grails-test-examples/app1

@jamesfredley are there already functional tests for the scaffolding plugin in those examples? Where exactly are you saying to put it?

@jdaugherty jdaugherty force-pushed the 7.0.x-genericServiceEnhancements branch from 380c117 to e48f971 Compare September 12, 2025 18:33
@jdaugherty
Copy link
Contributor

I took the app you created at https://github.com/codeconsole/scaffold-app and integrated it under grails-test-examples/scaffolding.

I added a test to invoke the user/index and it works correctly. community/user/index though is erroring:

image

@codeconsole
Copy link
Contributor Author

@jdaugherty
Copy link
Contributor

@jamesfredley can you please review?

@jdaugherty jdaugherty merged commit 73ff4ab into apache:7.0.x Sep 15, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants