Repository navigation
Conversation
Downstream SQL rewriters currently recover statement-leading comments from the JavaCC special-token chain via getASTNode(), whose erasure changed within 5.x and broke compiled downstream adapters. Signed-off-by: 付典 <fudianchn@gmail.com>
fudianchn
marked this pull request as draft
October 12, 2026 07:19
Keep the serial UID from 066f581 so existing serialized models remain readable. Return the original comment token images without line terminators. Signed-off-by: 付典 <fudianchn@gmail.com>
fudianchn
force-pushed
the
feat/statement-leading-comments
branch
from
October 12, 2026 07:39
0998415 to
7b08430
Compare
fudianchn
marked this pull request as ready for review
October 12, 2026 08:01
Contributor
There was a problem hiding this comment.
What is this one please?!
Contributor
Author
There was a problem hiding this comment.
The fixture was intended to verify that adding the accessor did not break deserialization of existing objects. I have removed the binary fixture.
| public class SerializationTest { | ||
| @Test | ||
| void deserializeSelectFromBeforeLeadingCommentsAccessor() throws Exception { | ||
| try (InputStream serialized = getClass().getResourceAsStream( |
Contributor
Author
There was a problem hiding this comment.
Replaced the fixture-based test with the existing in-memory round-trip helper, checking that the SQL is preserved and the deserialized node has no leading comments.
Contributor
Author
There was a problem hiding this comment.
I don't think this change needs to pin the base class's serialVersionUID. Fixing that value alone does not guarantee cross-version deserialization of the AST. I'll remove it and keep the test focused on same-version round trips.
Signed-off-by: 付典 <fudianchn@gmail.com>
fudianchn
marked this pull request as draft
October 13, 2026 02:28
fudianchn
marked this pull request as ready for review
October 13, 2026 02:32
fudianchn
marked this pull request as draft
October 13, 2026 03:09
Adding getLeadingComments changes the computed UID; previously serialized AST objects may no longer be readable. Signed-off-by: 付典 <fudianchn@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI disclosure: this change was prepared with AI coding agents.
What
ASTNodeAccessImpl.getLeadingComments()returns the comments preceding the node's first token, in source order, with the original token images.Why
PageHelper recovers statement-leading comments from the JavaCC special-token chain when rewriting SQL, using
select.getASTNode().jjtGetFirstToken().specialToken(DefaultCountSqlParserand theCountJSqlParser51adapter). This accessor provides a public entry point for that use case without exposing the token traversal to callers. ThegetASTNode()return-type change discussed in pagehelper-org/Mybatis-PageHelper#887 illustrates the cost of relying on parser internals; adopting this accessor requires downstream changes.Discussion #2018, converted from #2017, explains the ambiguity of assigning arbitrary comments to AST nodes. This accessor only reads existing comment tokens before a node's first token; it does not change SQL rendering.
How
One lazy accessor on
ASTNodeAccessImpl: walk the first token'sspecialTokenchain, collect the images, reverse to source order; nodes without an AST (built programmatically) return an empty list. No grammar, interface, deparser ortoString()changes.Testing
New
LeadingCommentsTest: block comments in source order (the exact shape PageHelper recovers,/* keep orderby */ /*+ FULL(t) */), line comments, mid-statement comments excluded, none, programmatic nodes,toString()unchanged, per-statement attribution underparseStatements.SerializationTestuses the existing in-memory round-trip helper to check SQL preservation and the empty leading-comment list after deserialization, when the transient AST is absent.Behavior notes
toString()and the deparsers keep dropping comments as before.ASTNodeAccessImpluses a computed serial UID, as before this PR. Adding the accessor changes that UID, so objects serialized with the previous class definition can fail to deserialize withInvalidClassException. Rebuild affected persisted AST caches after upgrading. Same-version round trips remain covered.