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