ENG-4729 by JohnBergant · Pull Request #120 · prime-framework/prime-mvc

JohnBergant · 2026-07-20T16:12:22Z

ENG-4729

- Add a value that sets the maximum amount of indexed values that can be parsed. - Add tests around the limit that is set. - Default the value to -1 that is unbounded (backwards compatible).

JohnBergant · 2026-07-20T16:16:23Z

+
+import org.primeframework.mvc.ErrorException;
+
+public class InvalidCollectionSizeException extends ErrorException {


Chose to override ErrorException here so we will return the associated response code in favor of implementing a custom handler for the exception.

Copilot

Pull request overview

This PR introduces a configurable bound on indexed collection/array parameter binding (e.g., foo[123]=bar) to prevent unbounded growth during expression evaluation, adds a dedicated exception for invalid sizes/indices, and updates tests and configuration to cover the new behavior.

Changes:

Add MVCConfiguration.collectionSizeLimit() and wire it into expression traversal, collection padding, and array/list creation.
Introduce InvalidCollectionSizeException and add test coverage for over-limit and negative indices.
Bump project version to 5.11.0.

Reviewed changes

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

Show a summary per file

File	Description
src/test/java/org/primeframework/mvc/parameter/el/DefaultExpressionEvaluatorTest.java	Adds coverage for maximum index enforcement, boundary behavior, and negative index rejection.
src/main/java/org/primeframework/mvc/parameter/el/InvalidCollectionSizeException.java	Adds a dedicated exception used when an indexed parameter exceeds configured bounds.
src/main/java/org/primeframework/mvc/parameter/el/IndexedCollectionAccessor.java	Enforces index bounds when padding lists/arrays for indexed writes.
src/main/java/org/primeframework/mvc/parameter/el/Expression.java	Passes configured collection limit into accessors and value creation.
src/main/java/org/primeframework/mvc/parameter/el/Accessor.java	Adds index validation for list/array creation paths and threads limit through value creation.
src/main/java/org/primeframework/mvc/config/MVCConfiguration.java	Introduces the new `collectionSizeLimit()` configuration surface.
src/main/java/org/primeframework/mvc/config/AbstractMVCConfiguration.java	Provides a default field-backed implementation of the new configuration value.
build.savant	Updates the library version.

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

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

bhalsey

This looks good! I think we should also have a couple .simulate(() -> simulator.test() tests.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

bhalsey · 2026-07-21T19:21:17Z

                .assertStatusCode(500));

-        // It will work if we use a backing collection with an iterator in the form to build multiple form fields
+        // Multiple values, 11 repeated params exceeds limit of 10, returns 400


what happened to this fix? the comment still says 400

Fixed up the comment and added more assertions.

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (3)

src/test/java/org/primeframework/mvc/GlobalTest.java:433

This comment says the request should return 400, but the test asserts 200. Given the PR description explicitly calls out repeated params as not constrained by the new limit, the comment should be corrected to match the actual expected behavior.

        // Multiple values, 11 repeated params exceeds limit of 10, returns 400

src/test/java/org/primeframework/mvc/GlobalTest.java:1970

This comment says the request should return 400, but the test asserts 200. Repeated params are described as not constrained by collectionSizeLimit, so the comment is misleading.

        // Multiple values, 12 repeated params exceeds limit of 10, returns 400

src/test/java/org/primeframework/mvc/GlobalTest.java:1988

These comments describe a "multiple values" scenario, but the test is actually validating an indexed parameter exceeding the configured limit. Updating the comment will make the intent clearer.

        // Multiple values, output contains these two values in a collection
        // should fail as the indexed array is larger than the "10" maximum

JohnBergant · 2026-07-21T14:31:37Z


  public boolean autoHTMLEscapingEnabled = true;

+  public int collectionSizeLimit = 10;


10 is the default for most cases which simplifies the instantiation for this config for the majority of instances.

JohnBergant · 2026-07-21T14:28:07Z

+public class InvalidCollectionSizeException extends ErrorException {
+  public InvalidCollectionSizeException(Object... args) {
+    super("input", args);
+  }


Updated the PR description.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (5)

src/test/java/org/primeframework/mvc/GlobalTest.java:1970

This comment says the request should return 400 when repeated parameters exceed the limit, but the test asserts 200. Per the PR description, repeated params are intentionally not constrained by collectionSizeLimit, so the comment should reflect that.

        // Multiple values, 12 repeated params exceeds limit of 10, returns 400

src/test/java/org/primeframework/mvc/GlobalTest.java:1989

The comment above the indexed-parameter test is misleading (it mentions "multiple values"/"two values" but the case is a single indexed parameter beyond the configured limit). Updating it will make the intent of the 400 assertion clearer.

        // Multiple values, output contains these two values in a collection
        // should fail as the indexed array is larger than the "10" maximum
        test.simulate(() -> simulator.test("/collection-converter")

src/test/java/org/primeframework/mvc/parameter/el/DefaultExpressionEvaluatorTest.java:43

MockConfiguration is imported but not referenced anywhere in this test file, which can trip unused-import checks and adds noise.

import org.primeframework.mvc.MockConfiguration;

src/main/java/org/primeframework/mvc/parameter/el/Accessor.java:165

The createValue JavaDoc says key is only used for arrays, but the method now also uses it to validate indexed collection/list creation. Updating the docs will prevent confusion for future maintenance.

   * @param key           This is only used when creating arrays. It is the next atom, which is always the size of the array.
   * @param maximumLength The configured maximum size of the collection, or {@code -1} for no limit.

src/test/java/org/primeframework/mvc/GlobalTest.java:437

The repeated-parameter case here asserts HTTP 200, but the comment says it should return 400. Also, this block no longer asserts that the repeated values were actually converted/rendered (it only checks the status), which weakens coverage for the "repeated params are not constrained" behavior described in the PR description.

        // Multiple values, 11 repeated params exceeds limit of 10, returns 400
        test.simulate(() -> simulator.test("/collection-converter")
                .withURLParameter("strings", "bar")
                .withURLParameter("strings", "baz")
                .withURLParameter("strings", "baa")

JohnBergant · 2026-07-21T21:33:41Z


-        // It will work if we use a backing collection with an iterator in the form to build multiple form fields
+        // Multiple values, 11 repeated params exceeds limit of 10, returns 400
        test.simulate(() -> simulator.test("/collection-converter")


Do we want this to behave differently? No limit on the URL parameters enforces request size restrictions bypassing the expansion issue but might cause confusion in the framework as it behaves differently.

Hmm. Yeah, it feels weird to allow more than the maximum collection size for repeated params but not indexing.

I spent some time on this last night and it seems like we'll need to enforce the size limit in the converters when they produce collections. This will add a bunch changes which isn't a bad thing, just something to keep in mind. This fix as it stands solves the expansion problem. To give an idea of the changes and the overall direction I had in mind: #121.

JohnBergant added 5 commits July 16, 2026 15:13

Add a maximum index for collections via forms

ca27967

- Add a value that sets the maximum amount of indexed values that can be parsed. - Add tests around the limit that is set. - Default the value to -1 that is unbounded (backwards compatible).

Add more context to java doc

5ec9453

Rename configuration key

2d71885

Create new exception that inherits collection exception

52cf2de

Update test for empty message

0cde10f

JohnBergant changed the title ~~Johnbergant/eng 4729 prime mvc unbounded parameter index causes oom on every~~ ENG-4729: Prime MVC unbounded parameter index Jul 20, 2026

JohnBergant commented Jul 20, 2026

View reviewed changes

JohnBergant requested a review from Copilot July 20, 2026 16:18

Copilot started reviewing on behalf of JohnBergant July 20, 2026 16:19 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread src/main/java/org/primeframework/mvc/config/MVCConfiguration.java Outdated

Comment thread src/main/java/org/primeframework/mvc/parameter/el/InvalidCollectionSizeException.java

JohnBergant and others added 2 commits July 20, 2026 10:46

Potential fix for pull request finding

d08d9df

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

Potential fix for pull request finding

e5a6c0d

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

bhalsey reviewed Jul 20, 2026

View reviewed changes

JohnBergant added 3 commits July 20, 2026 15:50

Add tests to cover the new changes around collections

be8a797

Update tests to be more comprehensive

693a520

Remove unused constructor

d0277d0

JohnBergant requested review from bhalsey and Copilot July 20, 2026 22:39

JohnBergant marked this pull request as ready for review July 20, 2026 22:39

Copilot started reviewing on behalf of JohnBergant July 20, 2026 22:40 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

JohnBergant and others added 7 commits July 20, 2026 16:55

Add default value test

07d2006

Add back the no limit tests

5eff19a

Potential fix for pull request finding

59583d3

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

Potential fix for pull request finding

954ca84

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

Potential fix for pull request finding

a336df9

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

Potential fix for pull request finding

2d8966f

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

Fix test

5d5afbf

JohnBergant requested a review from Copilot July 21, 2026 13:55

Copilot started reviewing on behalf of JohnBergant July 21, 2026 13:55 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

JohnBergant requested a review from Copilot July 21, 2026 19:29

Copilot started reviewing on behalf of JohnBergant July 21, 2026 19:30 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

JohnBergant commented Jul 21, 2026

View reviewed changes

JohnBergant added 2 commits July 21, 2026 16:06

nit: comment, import

40bad60

Update test comment and expand asserts

bedee75

JohnBergant changed the title ~~ENG-4729: Prime MVC unbounded parameter index~~ ENG-4729 Jul 22, 2026

JohnBergant closed this Jul 28, 2026


		import org.primeframework.mvc.ErrorException;

		public class InvalidCollectionSizeException extends ErrorException {


		public boolean autoHTMLEscapingEnabled = true;

		public int collectionSizeLimit = 10;

Uh oh!

Conversation

JohnBergant commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

bhalsey left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohnBergant commented Jul 20, 2026 •

edited

Loading