You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "borinquenkid (via GitHub)" <gi...@apache.org> on 2026/09/01 11:13:52 UTC
[PR] Fix/json scanner number parsing [grails-core]
borinquenkid opened a new pull request, #16286:
URL: https://github.com/apache/grails-core/pull/16286
## Description
JsonScanner's number-parsing state machine had two long-standing bugs where valid JSON numeric literals failed to parse:
1. A bare exponent-notation number at end-of-input (e.g. 1e2 with nothing after it) threw JsonParseException — the SAW_EXPONENT_DIGITS state was the only numeric state missing an explicit end-of-input
branch that its sibling states all had.
2. -Infinity failed to parse in every context, not just some — the character-matching loop appended one extra trailing character (the terminator right after the literal) into the buffer handed to
Double.parseDouble, so the parse always failed.
## Contributor Checklist
Please review the following checklist before submitting your pull request. Pull requests that do not meet these requirements may be closed without review.
### Issue and Scope
- [ ] This PR is linked to an existing issue that has been **acknowledged or approved** by the project team. If no approved issue exists, please give background on why this change is necessary. Tickets are preferred for release change log history.
- [ ] This PR addresses the **complete scope** of the linked issue. Partial implementations or unfinished work should not be submitted for review.
- [ X] This PR contains a **single, focused change**. Unrelated changes should be submitted as separate pull requests.
- [ X] This PR targets the **correct branch** for the type of change:
- **Patch release branches** (e.g., `7.0.x`): Bug fixes only. No new features or API changes.
- **Minor release branches** (e.g., `7.1.x`): New features are welcome, but breaking existing APIs must be avoided.
- **Major release branches** (e.g., `8.0.x`): Reserved for major changes. Breaking API changes are permitted.
### Code Quality
- [X ] I have **added or updated tests** that cover the changes introduced in this PR. All code contributions are expected to include appropriate test coverage.
- [ X] I have verified that all existing tests pass by running `./gradlew build --rerun-tasks`.
- [X ] My code follows the project's **code style** guidelines. I have run `./gradlew codeStyle` and resolved any violations. See [Code Style](../CONTRIBUTING.md#code-style) for details.
- [X ] This PR does **not** include mass reformatting, style-only changes, or large-scale refactoring unless it was **explicitly approved** in the linked issue. Unsolicited reformatting will not be accepted.
- [ X] If generative AI tooling was used in preparing this contribution, a quality model was used to ensure contributions are **consistent with the project's quality standards**.
### Licensing and Attribution
- [X ] All contributed code is provided under the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0), and new source files include the appropriate **Apache license header**.
- [ X] I have the necessary rights to submit this contribution and confirm it is my own original work (see [Legal Notice](../CONTRIBUTING.md#i-want-to-contribute)).
- [ X] If generative AI tooling was used in preparing this contribution, I have followed the [Apache Software Foundation's policy on generative tooling](https://www.apache.org/legal/generative-tooling.html) and have properly attributed its use.
### Documentation
- [ ] If this PR introduces user-facing changes, I have included or updated the relevant documentation.
- [ ] If this PR adds a new feature, I have updated the **What's New** section of the Grails Guide.
- [ ] If this PR introduces breaking changes or changes that require user action during an upgrade, I have updated the **Upgrade Notes** for the corresponding version in the Grails Guide.
- [ X] The PR description clearly explains **what** was changed and **why**.
---
> **First-time contributors:** Please read our [Contributing Guide](../CONTRIBUTING.md) before submitting.
> Pull requests that appear to be auto-generated, incomplete, or unrelated to an approved issue may be
> closed to help maintainers focus on reviewed and planned work. We appreciate your understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/json scanner number parsing [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid closed pull request #16286: Fix/json scanner number parsing
URL: https://github.com/apache/grails-core/pull/16286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/json scanner number parsing [grails-core]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16286:
URL: https://github.com/apache/grails-core/pull/16286#discussion_r3903661861
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
Review Comment:
`GitHubDeployRole` now attaches AWS managed policies including `AdministratorAccess-AWSElasticBeanstalk`, which substantially broadens the deployment role’s permissions beyond the previously-scoped inline policy. This increases blast radius if the role is ever assumed unexpectedly; consider reverting to least-privilege (or documenting why this breadth is required) and limiting permissions to the specific Elastic Beanstalk application/environments used by Forge.
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
AssumeRolePolicyDocument:
Statement:
- Action: sts:AssumeRoleWithWebIdentity
Condition:
StringEquals:
token.actions.githubusercontent.com:aud: sts.amazonaws.com
+ StringLike:
token.actions.githubusercontent.com:sub:
- Fn::Sub: repo:${GitHubRepository}:ref:refs/heads/${GitHubBranch}
+ - Fn::Sub: repo:${GitHubRepository}:ref:refs/heads/*.x
+ - Fn::Sub: repo:${GitHubRepository}:ref:refs/tags/v*
Review Comment:
The trust policy was broadened from a single configured branch to any `refs/heads/*.x` branch and any `refs/tags/v*` tag. This meaningfully expands which refs can assume the deployment role; if you only intend specific maintenance branches/tags, consider using an explicit allow-list (or reintroducing a parameter) to avoid granting deploy rights to unrelated `*.x` branches.
##########
grails-forge/infrastructure/README.md:
##########
@@ -93,4 +93,4 @@ aws cloudformation deploy \
HostedZoneId=<route53-hosted-zone-id>
```
-GitHub Actions assumes the shared stack's `DeployRoleArn`. Upload a normal JAR deployment ZIP to the exported artifact bucket, create an Elastic Beanstalk application version, then update one exported environment name. The trust policy is restricted to the configured repository and branch, and the deploy policy is restricted to the application, its versions, and the five declared slot environment names.
+GitHub Actions assumes the shared stack's `DeployRoleArn`. Upload a normal JAR deployment ZIP to the exported artifact bucket, create an Elastic Beanstalk application version, then update one exported environment name. The trust policy allows `apache/grails-core` maintenance branches matching `refs/heads/*.x` and tags matching `refs/tags/v*`. The deploy policy is restricted to the application, its versions, and the five declared slot environment names.
Review Comment:
This README sentence says the deploy policy is restricted, but `shared.yaml` now also attaches broad AWS managed policies to `GitHubDeployRole` (e.g. `AdministratorAccess-AWSElasticBeanstalk`). Update the wording so it accurately reflects the effective permissions granted by the role.
##########
grails-data-mongodb/bson/src/test/groovy/org/grails/datastore/bson/json/JsonReaderSpec.groovy:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.bson.json
+
+import org.bson.BsonType
+import spock.lang.Specification
+
+/**
+ * Regression coverage for two JsonScanner number-parsing bugs:
+ *
+ * <p>SAW_EXPONENT_DIGITS was the only numeric scanner state whose terminator switch omitted an
+ * explicit end-of-input case (every sibling state treats EOF the same as a closing delimiter), so
+ * a bare exponent-notation number with nothing following it (e.g. a top-level "1e2") threw
+ * JsonParseException instead of parsing.
+ *
+ * <p>SAW_MINUS_I appended the character it read on every iteration of its "-Infinity" match loop,
+ * including the terminator character read immediately after matching the literal's final 'y' - so
+ * the buffer handed to Double.parseDouble always carried one extra trailing character, and
+ * "-Infinity" failed to parse in every context (end-of-input, before a comma, before a closing
+ * bracket).
+ */
+class JsonReaderSpec extends Specification {
+
+ void "reads a bare exponent-notation number with nothing following it"() {
+ given:
+ JsonReader reader = new JsonReader('1e2')
+
+ expect:
+ reader.readBsonType() == BsonType.DOUBLE
+ reader.readDouble() == 100.0d
+ }
+
+ void "reads -Infinity as negative infinity, at end-of-input and followed by a delimiter"() {
+ expect:
+ new JsonReader('-Infinity').readBsonType() == BsonType.DOUBLE
+
Review Comment:
The new regression test for top-level `-Infinity` asserts the token type but not the parsed numeric value. Adding a `readDouble()` assertion here ensures the fix is exercised end-to-end for the end-of-input case (not just “doesn’t throw”).
##########
grails-forge/infrastructure/shared.yaml:
##########
@@ -229,14 +225,20 @@ Resources:
GitHubDeployRole:
Type: AWS::IAM::Role
Properties:
+ ManagedPolicyArns:
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess-AWSElasticBeanstalk
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkWebTier
+ - Fn::Sub: arn:${AWS::Partition}:iam::aws:policy/AWSElasticBeanstalkManagedUpdatesCustomerRolePolicy
Review Comment:
PR title/description focuses on `JsonScanner` number parsing, but this PR also changes Grails Forge AWS deployment infrastructure (CloudFormation role trust/permissions and the GitHub Actions deploy workflow). To keep the change set reviewable and release-notes-friendly, consider splitting the infrastructure updates into a separate PR or updating the PR description to explicitly cover these deployment changes and rationale.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/json scanner number parsing [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16286:
URL: https://github.com/apache/grails-core/pull/16286#issuecomment-5493664435
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16286?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 54.7292%. Comparing base ([`fc95f78`](https://app.codecov.io/gh/apache/grails-core/commit/fc95f7890655b5cfcaeafe58b310301a6a22492a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`070707e`](https://app.codecov.io/gh/apache/grails-core/commit/070707e354fbbe82aab511b5786371f27577cef2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 64 commits behind head on 8.0.x.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 8.0.x #16286 +/- ##
==================================================
+ Coverage 54.1360% 54.7292% +0.5932%
- Complexity 20257 20436 +179
==================================================
Files 2098 2101 +3
Lines 100920 100979 +59
Branches 17898 17908 +10
==================================================
+ Hits 54634 55265 +631
+ Misses 38501 37845 -656
- Partials 7785 7869 +84
```
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16286?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...vy/org/grails/datastore/bson/json/JsonScanner.java](https://app.codecov.io/gh/apache/grails-core/pull/16286?src=pr&el=tree&filepath=grails-data-mongodb%2Fbson%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fbson%2Fjson%2FJsonScanner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGEtbW9uZ29kYi9ic29uL3NyYy9tYWluL2dyb292eS9vcmcvZ3JhaWxzL2RhdGFzdG9yZS9ic29uL2pzb24vSnNvblNjYW5uZXIuamF2YQ==) | `46.8880% <100.0000%> (+10.2213%)` | :arrow_up: |
... and [55 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16286/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/json scanner number parsing [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16286:
URL: https://github.com/apache/grails-core/pull/16286#issuecomment-5494195559
## 🚨 TestLens detected 1 failed test 🚨
Here is what you can do:
1) Inspect the test failures carefully.
2) If you are convinced that some of the tests are flaky, you can mute them below.
3) Finally, trigger a rerun by checking the rerun checkbox.
### Test Summary
#### [Groovy Snapshot Canary Build / Build Grails \(shard 2\)](https://github.com/apache/grails-core/actions/runs/33501407024/job/99839434856?pr=16286) > :grails-test-examples-scaffolding:integrationTest
| Test | Runs | Flakiness |
|---|---|--:|
| UserControllerSpec > User list | ❌ | [3% 🟡](https://github.com/apache/grails-core/issues/16030) |
🏷️ Commit: 070707e354fbbe82aab511b5786371f27577cef2
▶️ Tests: 63581 executed
🟡 Checks: 76/81 completed
### Test Failures
<details><summary><strong>UserControllerSpec > User list</strong> (:grails-test-examples-scaffolding:integrationTest in <a href="https://github.com/apache/grails-core/actions/runs/33501407024/job/99839434856?pr=16286">Groovy Snapshot Canary Build / Build Grails (shard 2)</a>)</summary>
```
geb.waiting.WaitTimeoutException: condition did not pass in 30 seconds (failed with exception)
at geb.waiting.Wait.waitFor(Wait.groovy:128)
at geb.waiting.DefaultWaitingSupport.doWaitFor(DefaultWaitingSupport.groovy:55)
at geb.waiting.DefaultWaitingSupport.waitFor(DefaultWaitingSupport.groovy:41)
at geb.Page.waitFor(Page.groovy:120)
at com.example.pages.LoginPage.login(LoginPage.groovy:39)
at com.example.UserControllerSpec.User list(UserControllerSpec.groovy:48)
Caused by: Assertion failed:
title != pageTitle && $('input', name: 'username').empty
| | | |
| | | false
| | 'Please sign in'
| false
'Please sign in'
at com.example.pages.LoginPage.login_closure1(LoginPage.groovy:39)
at com.example.pages.LoginPage.login_closure1(LoginPage.groovy)
at geb.waiting.Wait.waitFor(Wait.groovy:117)
... 5 more
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
🔲 UserControllerSpec > User list <!-- uniqueId=[engine:spock]/[spec:com.example.UserControllerSpec]/[feature:$spock_feature_1_0] -->
Reuse successful test results:
🔲 ♻️ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
🔲 **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org