Skip to content

Seed groovy-json facade over org.grails.web.json#15958

Open
jamesfredley wants to merge 2 commits into
8.0.xfrom
feat/json-org-groovy-json-facade-seed
Open

Seed groovy-json facade over org.grails.web.json#15958
jamesfredley wants to merge 2 commits into
8.0.xfrom
feat/json-org-groovy-json-facade-seed

Conversation

@jamesfredley

Copy link
Copy Markdown
Contributor

Description

What was found

Problem Impact
org.grails.web.json is a vendored JSON.org-era fork Maintenance and security risk
Converters still depend on public facade types Cannot rip out internals in one step
Codebase 4.1 marked this strategic 8.x/9.0 work Needs a deprecation facade seed

What changed

Area Change
Facade Add GroovyJsonFacade bridging toward groovy-json
Public API Keep existing org.grails.web.json types
Docs Upgrade note with 9.0 removal plan
Tests Facade parse/serialize coverage

Out of scope / follow-up

Topic Status
Full internal rewrite of all converters Follow-up
Removing public JSONObject/JSONArray 9.0
Benchmarks vs current fork Follow-up

Related MD topics

Source Topic
Codebase 4.1 Vendored JSON.org-era fork

Contributor Checklist

Issue and Scope

  • Starter deprecation path.
  • Public API preserved.
  • Single focused seed.
  • Targets 8.0.x.

Code Quality

  • Tests added.
  • Focused web-common tests run.
  • No mass reformatting.
  • AI starting point labeled.

Licensing and Attribution

  • Apache License 2.0.
  • Contributor rights confirmed.
  • ai-generated-starting-point label applied.

Documentation

  • Upgrade note added.
  • Scope explained.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Introduce GroovyJsonFacade and begin deprecation path for the vendored JSON.org fork.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 seeds a GroovyJsonFacade in grails-web-common to start migrating Grails’ vendored org.grails.web.json JSON.org-era fork toward groovy-json, while keeping the existing JSONElement / JSONObject / JSONArray public facade types in place.

Changes:

  • Added GroovyJsonFacade to parse/serialize via groovy.json.JsonSlurper / JsonOutput while returning existing Grails JSON facade types.
  • Deprecated the internal JSONTokener as part of the planned removal path.
  • Added an upgrade note and initial Spock coverage for the new facade.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
grails-web-common/src/main/groovy/org/grails/web/json/GroovyJsonFacade.java Introduces the new groovy-json–backed facade to bridge toward replacing the internal JSON.org fork.
grails-web-common/src/main/groovy/org/grails/web/json/JSONTokener.java Deprecates the legacy tokener to steer usage toward the new facade path.
grails-web-common/src/test/groovy/org/grails/web/json/GroovyJsonFacadeSpec.groovy Adds initial tests for parsing/serialization via the new facade (needs adjustments per review comments).
grails-web-common/build.gradle Adds groovy-json dependency to support the new facade.
grails-doc/src/en/guide/upgrading/jsonGroovyJsonFacade.adoc Documents the migration seed and 9.0 removal intent.
grails-doc/src/en/guide/toc.yml Adds the new upgrading guide entry to the TOC.

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

Comment on lines +41 to +42
expect:
GroovyJsonFacade.toJson(object) == '{"name":"Grails","active":true}'
Comment on lines +34 to +35

void 'toJson renders existing JSONObject facade through groovy-json'() {
Comment on lines 51 to 53
api 'org.apache.groovy:groovy'
api 'org.apache.groovy:groovy-json'
api 'org.apache.groovy:groovy-templates'
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.1344%. Comparing base (b00e83e) to head (125bb6c).
⚠️ Report is 117 commits behind head on 8.0.x.

Files with missing lines Patch % Lines
...n/groovy/org/grails/web/json/GroovyJsonFacade.java 66.6667% 6 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15958        +/-   ##
==================================================
+ Coverage     49.5403%   51.1344%   +1.5941%     
- Complexity      16922      17617       +695     
==================================================
  Files            1999       2042        +43     
  Lines           93754      95517      +1763     
  Branches        16420      16591       +171     
==================================================
+ Hits            46446      48842      +2396     
+ Misses          40145      39382       -763     
- Partials         7163       7293       +130     
Files with missing lines Coverage Δ
...c/main/groovy/org/grails/web/json/JSONTokener.java 16.2651% <ø> (ø)
...n/groovy/org/grails/web/json/GroovyJsonFacade.java 66.6667% <66.6667%> (ø)

... and 135 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jdaugherty

Copy link
Copy Markdown
Contributor

Instead of using our vendored version we should just adopt the upstream version. I disagree with this change.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

Just to align on the goal - I think we actually want the same end state ("adopt upstream"), and the facade is the non-breaking route to it rather than an alternative.

Per the pre-release review (Codebase 4.1): grails-web-common carries a custom JSON stack - JSONObject.java (~1,191 lines), JSONArray, and a JavaCC-generated parser (JsonParserTokenManager.java, ~1,038 lines) - that predates groovy-json/Jackson and is a standing maintenance and security-surface cost (custom JSON parsers are where JSON CVEs live).

The recommended starting point there is exactly to move onto upstream groovy-json internally while keeping the public org.grails.web.json.* type surface as a deprecated facade:

  • groovy-json is already a BOM dependency, so this adds zero new deps.
  • The reason for a facade instead of a hard swap is backward compatibility: applications, plugins, and grails-converters (JSON.java) reference org.grails.web.json.JSONObject / JSONArray directly. Deleting that public API outright is a breaking change and isn't safe in 8.x.
  • So the facade is adopting upstream - it reimplements the internals on groovy-json and validates against the existing converter test suites in grails-test-suite-web - while deferring removal of the vendored public API to 9.0.

If your objection is specifically to keeping the vendored public API around at all, that's the 9.0 removal step this facade sets up. Would it help if I framed the PR explicitly as "reimplement internals on groovy-json; deprecate the public facade for 9.0 removal" so the upstream-adoption intent is unambiguous?

@jdaugherty

Copy link
Copy Markdown
Contributor

groovy-json has behavioral differences that has blocked me on trying this before and changing the json parsing stack is a major change. I'd rather we update to the successor of what was vendored, which in this case is openjson. It's a clean room implementation that is maintained, and it was driven by the ASF having problems with the original library. This maintains strict compatibility and is less likely to introduce regressions to behavioral differences.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@testlens-app

testlens-app Bot commented Jul 17, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 125bb6c
▶️ Tests: 13313 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants