Skip to content

feat: change Schema.required from List to Set (#5027)#5037

Open
yht0827 wants to merge 1 commit intoswagger-api:masterfrom
yht0827:feat/issue-5027-required-set
Open

feat: change Schema.required from List to Set (#5027)#5037
yht0827 wants to merge 1 commit intoswagger-api:masterfrom
yht0827:feat/issue-5027-required-set

Conversation

@yht0827
Copy link
Copy Markdown

@yht0827 yht0827 commented Jan 17, 2026

Pull Request

Thank you for contributing to swagger-core!

Please fill out the following information to help us review your PR efficiently.


Description

Change Schema.required field type from List<String> to Set<String> to prevent duplicate required properties.

  • Change internal storage from List to Set
  • Use TreeSet to maintain alphabetical ordering
  • Keep public API compatible (getRequired() still returns List)
  • Remove duplicates when adding required items
  • Add unit tests for duplicate handling and sorting

Fixes: #5027

Type of Change

  • ✨ New feature
  • ♻️ Refactor (non-breaking change)
  • 🧪 Tests

Checklist

  • I have added/updated tests as needed
  • The PR title is descriptive
  • The code builds and passes tests locally
  • I have linked related issues (if any)

Screenshots / Additional Context

스크린샷 2026-01-17 12 09 13

@ewaostrowska
Copy link
Copy Markdown
Contributor

Hi @yht0827,
Thanks for contributing to this repo and trying to make it better!
Unfortunately we cannot merge this PR in the current state since even though it would be an enhancement it would also introduce few risks:

  1. addRequiredItem() is not consistent with setRequired() :
    if properties is set and you do:
    setRequired(List.of("a", "notAProp"))"notAProp" will be dropped (and if nothing survives, required becomes null)
    addRequiredItem("notAProp") "notAProp" will be kept in required

They also differ in how they treat empty/null:

  • setRequired(null) clears
  • addRequiredItem(...) only adds.
  1. Behavior change in getRequired():
  • before, it returned the backing collection directly and after, it returns new ArrayList<>(required), a copy.
    So code like schema.getRequired().add("x") would previously mutate the schema, but after the PR it would only mutate the temporary returned list. That is a compatibility change even though the method still returns List
  1. Missing JavaDoc for getRequired() explaining that it returns a copy and why

@ewaostrowska ewaostrowska force-pushed the feat/issue-5027-required-set branch from 88e6aab to 74b6c87 Compare April 3, 2026 10:42
  - Change internal storage from List<String> to Set<String> using TreeSet
  - Keep public API compatible (getRequired() returns a defensive copy as List<String>)
  - Make addRequiredItem() consistent with setRequired(): ignore items not present in properties
  - Add JavaDoc to getRequired() explaining the defensive copy behavior
  - Add unit tests for duplicate handling, sorting, and consisteny
@yht0827 yht0827 force-pushed the feat/issue-5027-required-set branch from ff8fc26 to d9cf8b5 Compare April 3, 2026 15:36
@yht0827
Copy link
Copy Markdown
Author

yht0827 commented Apr 3, 2026

Hi @ewaostrowska, thank you for the detailed review!

I've addressed all the feedback:

  1. addRequiredItem() consistency: Added the same properties check as setRequired() — items not present in properties are now ignored.
  2. getRequired() compatibility: Added JavaDoc explicitly documenting that the method returns a defensive copy and that mutating the returned list does not affect the schema.
  3. Tests: Added test cases covering the new addRequiredItem() behavior and the defensive copy behavior of getRequired().

Please take another look when you have a chance. Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"required" property in Schema.java should be a Set (SortedSet for example), not a List

3 participants