Skip to content

Conversation

@Noirtam
Copy link
Contributor

@Noirtam Noirtam commented Nov 13, 2025

Remove add contextPath linkGenerator.contextPath but fail to test Issue #11673

@sbglasius sbglasius requested a review from matrei November 13, 2025 14:02
@sbglasius
Copy link
Contributor

@matrei I would like a second pair of eyes on this. Thank you.

@Noirtam
Copy link
Contributor Author

Noirtam commented Nov 13, 2025

@sbglasius, Thank you for the approval.

Are you OK with the revert on previous fix (7875fe4) ? I think it was a misuse of the redirection with absolute=false . The old test could be remove

@jdaugherty
Copy link
Contributor

So it looks like #11673 is what introduced this behavior. That code however added tests that have not been run against this PR yet. I've gone ahead and approved the test run to see if they pass.

@jamesfredley jamesfredley moved this to In Progress in Apache Grails Nov 18, 2025
@jamesfredley jamesfredley added this to the grails:7.0.3 milestone Nov 18, 2025
sbglasius
sbglasius previously approved these changes Nov 19, 2025
@matrei
Copy link
Contributor

matrei commented Nov 19, 2025

@Noirtam I don't think we should remove the existing test. It was put there to test another scenario.

@Noirtam
Copy link
Contributor Author

Noirtam commented Nov 19, 2025

@matrei I may not have understood the previous issue.
I tried to add a new solution to the earlier problem and pass both the existing test and my new test.

@sbglasius sbglasius self-requested a review November 19, 2025 15:59
@sbglasius sbglasius dismissed their stale review November 19, 2025 16:00

I need more time to test this in different scenarios

@sbglasius
Copy link
Contributor

@jdaugherty here's what I did to test:

Checked out https://github.com/Noirtam/grails-core/tree/issues_15132 and build according to INSTALL

Added to application.yml

---
server:
  servlet:
    context-path: /demo

Created two controllers:

package DemoApplication

class TestAlphaController {
    
    def index() {
        redirect(controller: 'testBeta', action: 'index')
    }
}

and

package DemoApplication

class TestBetaController {
    
    def index() {
        render 'OK'
    }
}

First I ran the application with ./gradlew bootRun and saw that it started on http://localhost:8080/demo, clicked on http://localhost:8080/demo/testApha and was redirected to /demo/testBeta and OK was rendered.

Secondly I build ./gradlew war and deployed that to a vanilla Tomcat 10, by placing the created warfile in webapps/demo.war and fired up tomcat.

Again I did the same test, and everything worked

Finally I stopped tomcat, removed webapps/demo and renamed demo.war to test.war (new context), and fired up tomcat.

Now I hit http://localhost:8080/test, clicked on http://localhost:8080/test/testApha and was redirected to /test/testBeta and OK was rendered.

For extra testing I build ./gradle bootJar and ran that:

java -jar build/libs/DemoApplication-0.1.jar    

and secondly with a different context-path:

java -Dserver.servlet.context-path=/test -jar build/libs/DemoApplication-0.1.jar 

Also both worked as expected.

My conclusion is, that the patch works as expected in all scenarios

Copy link
Contributor

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

Thorough testing concludes that this change works as expected.

@sbglasius
Copy link
Contributor

@jdaugherty
Copy link
Contributor

Once codenarc is fixed, we can merge

@jdaugherty
Copy link
Contributor

@Noirtam Can you please update your PR to adhere to our code style? You can run ./gradlew codeStyle locally to generate the code narc report so you know what to fix.

@jdaugherty jdaugherty modified the milestones: grails:7.0.3, grails:7.0.4 Nov 20, 2025
@jdaugherty
Copy link
Contributor

I've moved this to 7.0.4 with the hope that the code style is updated for that release. We'll likely release 7.0.4 soon after 7.0.3 due to the spring boot update that's due tomorrow.

@Noirtam
Copy link
Contributor Author

Noirtam commented Nov 20, 2025

@sbglasius Thank you for fixing the whitespace issue.

@jdaugherty jdaugherty merged commit 6a10193 into apache:7.0.x Nov 20, 2025
35 of 36 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Apache Grails Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants