tyge68 commented on PR #45: URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/45#issuecomment-4160871394
Suggestion / possible follow-up In SelectedFieldWrapper’s constructor, subFieldMap.put(sf.getName(), selectedChildField) runs before the duplicate-FQN merge. When two children share the same FQN, the second wrapper is merged into the first but may still remain in subFieldMap under the same simple name, so getSubSelectedFieldByName / hasDuplicateFieldByName could still see two entries (W1 merged + W2 merged-away). If callers iterate by simple name, they might hit a redundant wrapper. Consider deferring the subFieldMap put until you know the FQN is new, or removing the merged-away wrapper from subFieldMap after merge. Docs / nit Class-level Javadoc still says sub-fields are assumed unique by FQN (and “unqiue” → “unique”); worth a short note that duplicate FQNs are now merged. The README-only “trigger build” commit might be worth squashing or dropping before merge. No blockers from my side if the subFieldMap behavior is acceptable for current API consumers; otherwise the follow-up above would tighten consistency. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
