Add support for convering keys to lowercase/uppercase in RenameKeyProcessor#5810
Merged
kkondaka merged 3 commits intoopensearch-project:mainfrom Jun 24, 2025
Merged
Conversation
…cessor Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
dab2e10 to
09d6495
Compare
san81
reviewed
Jun 23, 2025
Comment on lines
+45
to
+49
| boolean entriesProvided = config.getEntries() != null; | ||
| boolean transformOptionProvided = (transformOption != null && transformOption != TransformOption.NONE); | ||
| if ((!entriesProvided ^ transformOptionProvided)) { | ||
| throw new InvalidPluginConfigurationException("Only one of 'entries' or 'transform_key' should be specified."); | ||
| } |
Collaborator
There was a problem hiding this comment.
I think, it is Ok to have this logic here but we should also have this validation in the RenameKeyProcessorConfig so that it gets caught early on.
dlvenable
requested changes
Jun 23, 2025
|
|
||
| boolean entriesProvided = config.getEntries() != null; | ||
| boolean transformOptionProvided = (transformOption != null && transformOption != TransformOption.NONE); | ||
| if ((!entriesProvided ^ transformOptionProvided)) { |
Member
There was a problem hiding this comment.
Let's be consistent by adding the assertions in the config files.
e.g.
| throw new InvalidPluginConfigurationException("Only one of 'entries' or 'transform_key' should be specified."); | ||
| } | ||
| if (config.getEntries() == null || config.getEntries().size() == 0) { | ||
| return; |
Member
There was a problem hiding this comment.
This could be error prone. It is assuming that no code below does anything else of importance.
Instead:
if(config.getEntries != null) {
config.getEntries().forEach(...
I do think short-circuit returns are good for functions. But, not for constructors which operate differently.
|
|
||
| private void transformEvent(final Event event, Map<String, Object> map, final String keyPrefix) { | ||
| for (Map.Entry<String, Object> entry : map.entrySet()) { | ||
| Object result = null; |
| assertThat(editedRecords.get(0).getData().containsKey("key2"), is(true)); | ||
| assertThat(editedRecords.get(0).getData().containsKey("key2/key3"), is(true)); | ||
| assertThat(editedRecords.get(0).getData().containsKey("key2/key3/key4"), is(true)); | ||
| assertThat(editedRecords.get(0).getData().containsKey("key2/key3/key5"), is(true)); |
Member
There was a problem hiding this comment.
Also assert that the old keys are not present.
san81
reviewed
Jun 23, 2025
Collaborator
There was a problem hiding this comment.
Can we have one more test case to validate the logic in (!entriesProvided ^ transformOptionProvided) Like to test this validation part
Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
san81
previously approved these changes
Jun 24, 2025
dlvenable
requested changes
Jun 24, 2025
|
|
||
| ReflectivelySetField.setField(RenameKeyProcessorConfig.class, renameKeyProcessorConfig, "entries", entries); | ||
| ReflectivelySetField.setField(RenameKeyProcessorConfig.class, renameKeyProcessorConfig, "transformOption", TransformOption.LOWERCASE); | ||
| assertFalse(renameKeyProcessorConfig.isValidConfig()); |
Member
There was a problem hiding this comment.
These should all be separate @Test methods because they are different tests.
Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>
dlvenable
approved these changes
Jun 24, 2025
san81
approved these changes
Jun 24, 2025
JonahCalvo
pushed a commit
to JonahCalvo/os-data-prepper
that referenced
this pull request
Jul 17, 2025
…cessor (opensearch-project#5810) * Add support for convering keys to lowercase/uppercase in RenameKeyProcessor Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> Signed-off-by: Jonah Calvo <caljonah@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add support for convering keys to lowercase/uppercase in RenameKeyProcessor.
Reused existing TransformOption in KeyValue processor in RenameKeyProcessor.
Addresses part of the issue #5757
Issues Resolved
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.