fix: ensure accented characters are sorted with their base letters #12284
Conversation
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
5 times, most recently
from
b347d55 to
4f3a8d1
Compare
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
3 times, most recently
from
5a0842a to
dc34e37
Compare
| if (value == null) return new BytesRef(""); | ||
| collator.setStrength(Collator.SECONDARY); | ||
| CollationKey key = collator.getCollationKey(value); | ||
| return new BytesRef(key.toByteArray()); | ||
| } |
There was a problem hiding this comment.
This seems to no longer handle special characters.
Can you handle when there are special characters and add a test that makes sure they are handled appropriately?
| private String normalizeForSorting(String value) { | ||
| if (value == null) return ""; | ||
| collator.setStrength(Collator.SECONDARY); | ||
| CollationKey key = collator.getCollationKey(value); | ||
| return Base64.getEncoder().encodeToString(key.toByteArray()); | ||
| } |
There was a problem hiding this comment.
This is quite similar to the toSortedValue. Can you mutualize the shared functionalities?
| @@ -231,7 +237,7 @@ private void appendPath(final Document doc, final int[] pathIndex, final String | |||
| doc.add(new StringField(FIELD_PATHS, path, Field.Store.NO)); | |||
| doc.add(new TextField(FIELD_PATHS_SPLIT, path, Field.Store.NO)); | |||
| if (pathIndex[0]++ == 0) { | |||
| doc.add(new SortedDocValuesField(FIELD_PATHS_SORTED, new BytesRef(QueryParser.escape(path)))); | |||
| doc.add(new SortedDocValuesField(FIELD_PATHS_SORTED, new BytesRef(normalizeForSorting(path)))); | |||
There was a problem hiding this comment.
The QueryParser.escape handled special characters. So this would also need to be done in your fix 👍
| @Test | ||
| void should_sort_names_by_bytesref() throws Exception { | ||
| List<String> names = List.of("nano", "Zorro", "Äther", "VEM", "épée", "Épona", "Öko", "BNS"); | ||
| List<String> expectedSorted = List.of("Äther", "BNS", "épée", "Épona", "nano", "Öko", "VEM", "Zorro"); |
There was a problem hiding this comment.
I believe these should all be lowercase. The name field is sorted by lowercase names in the IndexableApiDocumentTransformer 🤔
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
2 times, most recently
from
5de3d20 to
9df8998
Compare
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
from
9df8998 to
eae1d89
Compare
|
|
||
| @Test | ||
| public void shouldSortListCorrectlyWithCollatorAndBytesRef() throws Exception { | ||
| List<String> names = List.of("nano", "zorro", "äther", "vem", "foo/bar", "épée", "épona", "öko", "bns-one"); |
There was a problem hiding this comment.
WDYT of passing names with capital letters in it to see that the toLowerCase works as expected? (same for IndexableApiDocumentTransformerTest.java tests)
There was a problem hiding this comment.
Hi @jourdiw
I updated the test case to show mix behaviour (upper case, lower case and special chars) and the reason is that we don't need specifically lower case because now, collator is being used and it is case-insensitive but it will sort the names in right order now.
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
from
eae1d89 to
9895483
Compare
vikrantgravitee
force-pushed
the
APIM-9910-fix-apis-sorting
branch
from
9895483 to
36b201b
Compare
Issue
https://gravitee.atlassian.net/browse/APIM-9910
Description
Now, API names are sorted using locale-aware comparison, ensuring correct collation.
Fix:
api_sort.mov
Additional context