Fix getHoldingsSummary returning bad counts when too many contracts

[OriolMunoz-da]

Fixes #1061

[Copilot]

Pull Request Overview

This PR addresses an issue where getHoldingsSummary returns incorrect contract counts when too many contracts are present by enforcing a hard limit and using NonEmptyVector for party identifiers. Key changes include:

• Updating test cases in AcsSnapshotStoreTest.scala to use NonEmptyVector instead of Seq.
• Modifying AcsSnapshotStore.scala to use a stricter limit check via applyLimitOrFail.
• Updating HttpScanHandler.scala and the OpenAPI spec for proper input validation, and introducing nonEmptyOrFail.
• Introducing documentation and refactoring in the Limit.scala helper.

Reviewed Changes

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

Show a summary per file

File	Description
apps/scan/src/test/scala/org/lfdecentralizedtrust/splice/store/db/AcsSnapshotStoreTest.scala	Updates test case inputs and adds a test to assert failure when exceeding HardLimit.
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/AcsSnapshotStore.scala	Refactors limit handling through applyLimitOrFail and updates partyIds usage.
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/admin/http/HttpScanHandler.scala	Improves input validation with nonEmptyOrFail and updates partyIds conversion.
apps/scan/src/main/openapi/scan.yaml	Adds minItems validation to ensure non-empty party id arrays.
apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/Limit.scala	Introduces applyLimitOrFail for enforcing hard limits and includes more descriptive error handling.

Comments suppressed due to low confidence (3)

apps/scan/src/test/scala/org/lfdecentralizedtrust/splice/store/db/AcsSnapshotStoreTest.scala:582

• [nitpick] Consider rephrasing the test failure message to improve clarity, e.g., 'Unexpected success: expected an exception due to exceeding the hard limit.'

yield fail("should not get here, call should've failed")

apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/admin/http/HttpScanHandler.scala:1393

• Consider adding unit tests for nonEmptyOrFail to ensure it correctly throws an exception when provided with an empty vector.

private def nonEmptyOrFail[A](fieldName: String, vec: Vector[A]): NonEmptyVector[A] = {

apps/common/src/main/scala/org/lfdecentralizedtrust/splice/store/Limit.scala:92

• [nitpick] Consider adding inline documentation for applyLimitOrFail to clarify its behavior, particularly regarding the HardLimit condition and its exception throwing.

protected final def applyLimitOrFail[CC[_], C](name: String, limit: Limit, result: C & scala.collection.IterableOps[?, CC, C]): C = {

[OriolMunoz-da]

technically a breaking change but:

• no changes to scala code that i've seen
• it wasn't doing anything useful anyway

[isegall-da]

The only change is that we reject empty party ID lists, right? Seems fine. But let's mention it in the release notes.

[isegall-da]

Nice, thank you

[rautenrieth-da]

Thanks!

[rautenrieth-da]

Why do you need the fail() part, instead of something like yield succeed? Would .failed.futureValue not fail the test with a useful description if it didn't throw an exception?

[OriolMunoz-da]

I guess it doesn't matter much what I put in the yield branch - seems more explicit than just a succeed