Skip to content

fix: reject unsupported JSP/Groovy declaration blocks in GSP parser#15398

Open
jamesfredley wants to merge 1 commit intoapache:7.0.xfrom
jamesfredley:fix/gsp-declaration-block-error
Open

fix: reject unsupported JSP/Groovy declaration blocks in GSP parser#15398
jamesfredley wants to merge 1 commit intoapache:7.0.xfrom
jamesfredley:fix/gsp-declaration-block-error

Conversation

@jamesfredley
Copy link
Contributor

Summary

GSP declaration blocks (<%! ... %> and !{ ... }!) have never worked correctly in any Grails version. The declare() method in GroovyPageParser.java generates broken Groovy code that causes a 500 Internal Server Error at runtime with no useful diagnostic. This fix replaces the broken implementation with a clear compile-time GrailsTagException that tells the developer exactly what syntax is unsupported and suggests alternatives.

Bug Description

When a GSP contains a JSP-style declaration block:

<%! int counter = 0; %>

The application compiles and starts normally, but rendering the page at runtime produces a 500 Internal Server Error. The error message gives no indication that the declaration block syntax is the cause.

Before (broken — 500 error, no diagnostic):

org.codehaus.groovy.runtime.InvokerInvocationException: ...
  Caused by: groovy.lang.MissingPropertyException: No such property: counter

After (clear compile-time error):

org.grails.taglib.GrailsTagException: JSP-style declaration blocks (<%! ... %>) are not supported
in Groovy Server Pages. Use <% ... %> for scriptlet code or move logic to a controller or service.

Root Cause

The GSP parser (GroovyPageParser.java) uses a two-pass system:

  • Pass 1 (finalPass=false): Pre-scan. Most handlers skip via if (!finalPass) return;
  • Pass 2 (finalPass=true): Code generation. This is where the class body is written

The declare() method uniquely has inverted logic — if (finalPass) return; — so it runs during Pass 1 only. This causes declaration content to be written to the output stream before the class definition, between the package statement and class declaration:

// Generated code (broken):
package com.example.demo
int counter = 0;           // ← written by declare() in Pass 1
class DemoPage extends GroovyPage {
    void run() { ... }     // ← written in Pass 2
}

This creates an invalid Groovy script+class hybrid. Groovy compiles it without error, but at runtime the class body cannot see the script-level variable, causing MissingPropertyException.

This has been broken since the very first commit in Grails 1.1 (March 2009). The declare() method has never generated correct code. Zero real-world usage exists — searching all public GitHub repositories for <%! in .gsp files returns exactly 1 result (a Sublime Text syntax highlighting test file). The Groovy variant !{ ... }! has 0 results.

Fix

Replaced the broken declare() implementation with a GrailsTagException throw, following the same error pattern used throughout GroovyPageParser.java:

// Before (broken since Grails 1.1):
private void declare(boolean gsp) {
    if (finalPass) {
        return;
    }
    out.println();
    write(scan.getToken().trim(), gsp);
    out.println();
    out.println();
}

// After (clear error):
private void declare(boolean gsp) {
    String syntax = gsp ? "!{ ... }!" : "<%! ... %>";
    throw new GrailsTagException(
            "JSP-style declaration blocks (" + syntax + ") are not supported in Groovy Server Pages. " +
            "Use <% ... %> for scriptlet code or move logic to a controller or service.",
            pageName, getCurrentOutputLineNumber());
}

The error is thrown during parsing (before any code generation), so developers get immediate feedback with the exact file name, line number, and a suggestion for the correct alternative syntax.

Files Changed

File Change
grails-gsp/core/.../GroovyPageParser.java Changed declare() to throw GrailsTagException instead of writing broken code
grails-gsp/plugin/.../ParseSpec.groovy Added 3 Spock tests for declaration block rejection

Test Coverage

3 new Spock tests added to ParseSpec.groovy (all 13 tests pass — 10 existing + 3 new):

  1. parse with JSP declaration block throws error — verifies <%! int counter = 0; %> throws GrailsTagException with message containing "declaration blocks", "<%! ... %>", and "not supported"
  2. parse with JSP declaration block containing method throws error — verifies <%! String hello() { return "hi"; } %> embedded in HTML throws GrailsTagException
  3. parse with Groovy declaration block throws error — verifies !{ int counter = 0; }! (Groovy variant) throws GrailsTagException with message containing "!{ ... }!"

Example Application

Repository: https://github.com/jamesfredley/grails-gsp-declaration-block-bug

Grails 7.0.7 app with two pages:

  1. http://localhost:8080/bugDemo/safe — uses <% int counter = 0; %> (scriptlet) — renders correctly
  2. http://localhost:8080/bugDemo/index — uses <%! int counter = 0; %> (declaration block) — 500 error (demonstrates the bug)
./gradlew bootRun
# Visit http://localhost:8080/bugDemo/safe   → works
# Visit http://localhost:8080/bugDemo/index  → 500 error (bug)

Environment Information

  • Grails: 7.0.7
  • GORM: 7.0.7
  • Spring Boot: 3.5.10
  • Groovy: 4.0.30
  • JDK: 17 (Amazon Corretto)

Version

7.0.7

GSP declaration blocks (<%! ... %> and !{ ... }!) have never worked
correctly — the declare() method wrote content before the class
definition during pass 1, creating a broken script+class hybrid that
fails at runtime with a 500 error and no useful diagnostic.

Replace the broken declare() implementation with a clear
GrailsTagException that tells the developer exactly what syntax is
unsupported and suggests alternatives (<% %>, controller, or service).

Fixes: <%! int x = 0; %> now fails at parse time with a descriptive
error instead of silently generating broken bytecode.

- Change declare() in GroovyPageParser to throw GrailsTagException
- Add 3 Spock tests covering JSP (<%! %>), JSP with method, and
  Groovy (!{ }!) declaration block variants
- All 13 ParseSpec tests pass (10 existing + 3 new)
}

private void declare(boolean gsp) {
if (finalPass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the final pass check?

Copy link
Contributor Author

@jamesfredley jamesfredley Feb 18, 2026

Choose a reason for hiding this comment

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

The finalPass guard is irrelevant because the new code unconditionally throws a GrailsTagException. Since declare() is called during pass 1 (when finalPass=false), the exception fires immediately on the first encounter. There's no reason to let pass 1 succeed and then throw on pass 2 - the guard would be dead code after the throw.

@bito-code-review
Copy link

The final pass check was removed to change the behavior of the declare method. Previously, it would skip processing during the final pass, but now it unconditionally throws an exception to disable support for JSP-style declaration blocks in Groovy Server Pages.

}

out.println();
write(scan.getToken().trim(), gsp);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like the right change to me. This was throwing an error before - are you saying this declare worked in previous versions? @davydotcom did rework some of this in 7.x, is this really just a bug introduced in 7.x?

Copy link
Contributor Author

@jamesfredley jamesfredley Feb 18, 2026

Choose a reason for hiding this comment

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

The old declare() was not throwing an error. It was calling write(scan.getToken().trim(), gsp) during pass 1, which silently emits content to the output stream. Because pass 2 (where the class body is generated) hasn't run yet, this content ends up placed between the package statement and the generated class declaration:

// Generated code (broken):
package com.example.demo
int counter = 0;           // written by declare() in pass 1
class DemoPage extends GroovyPage {
    void run() { ... }     // written in pass 2
}

Groovy compiles this without error (it's a valid script+class hybrid), but at runtime the class body can't see the script-level variable, causing a MissingPropertyException (500 error with no diagnostic pointing to the declaration block).

This is not a 7.x regression

The declare() method body is identical to the original 2009 commit (3e17890a22). David Estes's 7.x optimization commit (85148d4612, Oct 2025) only touched the class javadoc and renamed printHtmlPart( to h( in htmlPartPrintlnRaw() - he did not touch declare() or the two-pass logic at all.
The core issue is that declare() has inverted finalPass logic compared to every other code-generating method in the parser:

Method Guard Runs in Purpose
script() if (!finalPass) return Pass 2 Code generation
expr() if (!finalPass) return Pass 2 Code generation
html() if (!finalPass) return Pass 2 Code generation
scriptletExpr() if (!finalPass) return Pass 2 Code generation
startTag() if (!finalPass) return Pass 2 Code generation
endTag() if (!finalPass) return Pass 2 Code generation
direct() if (finalPass) return Pass 1 Directive processing (correct - configures parser state)
declare() if (finalPass) return Pass 1 Writes code output (broken - ends up before class definition)

declare() writes code like script() does, but runs in pass 1 like direct() does. That mismatch is the bug, and it has existed since Grails 1.1 (March 2009).

Copy link
Contributor

Choose a reason for hiding this comment

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

have you actually tested that this is an issue in Grails 6 though? This looks like an AI response and I want to understand that we actually created an app to ensure this isn't really a borken feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With https://github.com/jamesfredley/grails-gsp-declaration-block-bug it can be reproduced. The reason this was not brought up for fixed before is likely that <%! int counter = 0; %> has been a known anti-pattern for a long time in JSP and most user didn't try it in GSP because it is not a good idea. This is a super edge case, but does produce a runtime exception if you do it.

Copy link
Contributor

@jdaugherty jdaugherty Feb 19, 2026

Choose a reason for hiding this comment

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

The app you linked is a Grails 7 app though. Is there a branch where you tested this with Grails 6?

Copy link
Contributor

Choose a reason for hiding this comment

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

Corrected my comment, I originally said Grails 6 instead of Grails 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not, but that code has been present since Grails 1.1, so the outcome should be the same. Users in general know <%! int counter = 0; %> is a bad idea, but if they try it, it compiles but then errors during runtime. This PR is just trying to alert them during the build instead of at runtime. The outcome is the same, an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

My fear in this area is the GSP dependency structure was significantly changed in Grails 7 and the area needs a lot of work. We should first confirm this isn't a regression before removing it.

@jamesfredley jamesfredley marked this pull request as draft February 17, 2026 19:58
@jamesfredley jamesfredley marked this pull request as ready for review February 18, 2026 23:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the GSP compiler to explicitly reject JSP/Groovy declaration block syntaxes that historically generated invalid Groovy output and caused confusing runtime failures, replacing them with a clear compile-time GrailsTagException.

Changes:

  • Replace GroovyPageParser.declare() codegen with an explicit GrailsTagException explaining the unsupported syntax and suggested alternatives.
  • Add Spock coverage ensuring both <%! ... %> and !{ ... }! declaration blocks are rejected during parsing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java Throws a compile-time GrailsTagException when declaration blocks are encountered instead of generating broken Groovy output.
grails-gsp/plugin/src/test/groovy/org/grails/web/pages/ParseSpec.groovy Adds tests asserting parser rejection and message content for both declaration block syntaxes.
Comments suppressed due to low confidence (1)

grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:470

  • The error message always says "JSP-style declaration blocks" even when gsp is true (syntax !{ ... }!). That wording is contradictory/misleading for the Groovy/GSP form. Consider making the wording conditional (e.g., "GSP-style" for !{...}!) or using a neutral phrase like "Declaration blocks" for both forms.
        String syntax = gsp ? "!{ ... }!" : "<%! ... %>";
        throw new GrailsTagException(
                "JSP-style declaration blocks (" + syntax + ") are not supported in Groovy Server Pages. " +
                "Use <% ... %> for scriptlet code or move logic to a controller or service.",
                pageName, getCurrentOutputLineNumber());

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Nice one!

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants

Comments