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

[yht0827]

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

[ewaostrowska]

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.

2. 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

3. Missing JavaDoc for getRequired() explaining that it returns a copy and why

[yht0827]

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!

[daniel-kmiecik]

@yht0827 thank you for the contribution.

However, this PR introduces two silent breaking changes that we'd like to avoid outside of a major release:

• getRequired() now returns a defensive copy. Any code doing schema.getRequired().add("x") will silently stop affecting the schema.
• required(List) fluent builder now filters by properties. Previously a raw field assignment, it now delegates to setRequired(), which silently drops items not present in properties. Callers using the builder after setProperties() may lose required fields with no error or warning.

Since we're not planning a new major release in the near term, we've prepared a non-breaking alternative in #5163 that achieves the same deduplication goal by keeping List internally and adding contains() guards in addRequiredItem(), setRequired(), and the required(List) builder (zero API impact, no behavioral regression for existing callers).

Your work was a great starting point and helped us identify and reason through the problem clearly. We really appreciate the effort you put into this. If you feel the SortedSet-based approach is still worth pursuing for a future major release, feel free to leave this PR open. Otherwise, feel free to close it.

[yht0827]

Thank you for the thorough explanation, @daniel-kmiecik. I'll close this PR since the team has a non-breaking alternative ready. Happy to have contributed to identifying the problem!