agturley commented on code in PR #11355:
URL: https://github.com/apache/nifi/pull/11355#discussion_r3456189851


##########
nifi-extension-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/PutElasticsearchJson.java:
##########
@@ -241,6 +241,11 @@ public class PutElasticsearchJson extends 
AbstractPutElasticsearch {
             .name("Identifier Field")
             .description("""
                     The name of the field within each document to use as the 
Elasticsearch document ID. \
+                    A nested field can be referenced with a "/"-delimited 
path: for a document \

Review Comment:
   So I started on this and ran into something I want to flag before going 
further.
   
   The problem is migrateProperties runs on every flow import and restart, and 
there's no way to tell a 2.10 literal field name like @metadata/id apart from a 
2.11 nested path @metadata/id — they're the exact same string. So if I just 
escape slashes, it also escapes legit nested paths and breaks them. I actually 
hit this on my test cluster: imported a normal 2.11 flow and every nested-path 
case quietly stopped working because the migration had rewritten @metadata/id 
to @metadata\/id on import.
   
   The only way I found to make it safe is to rename the three properties 
(Identifier Field → Identifier Field Path, etc). Then the old name only exists 
in pre-2.11 flows, so I can escape values found under the old name and leave 
the new ones alone. That works, but it means renaming three properties 
everyone's already using.
   
   This is feeling like a lot for the case we're protecting against — a 2.10 
flow that happens to have a literal / in the field name, and slashes aren't 
really common in ES field names to begin with. So I'm inclined to skip the 
migration and just document it: on upgrade, a field name with a literal / gets 
read as a nested path now, and you can escape it as \/ if you want the old 
behavior.
   
   Let me know if you'd rather I do the rename though, happy to if you think 
the upgrade guarantee is worth it.



-- 
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]

Reply via email to