You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/12/08 03:07:14 UTC
[GitHub] [nifi] NissimShiman opened a new pull request, #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
NissimShiman opened a new pull request, #6770:
URL: https://github.com/apache/nifi/pull/6770
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-10853](https://issues.apache.org/jira/browse/NIFI-10853)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1382068327
Thank you very much @mattyb149 for looking at this!
You raise a very good point about why we don't have an EL validator for non-stateful mode.
This issue started when stateful was added [NIFI-1582](https://issues.apache.org/jira/browse/NIFI-1582), as part of a large
[commit](https://github.com/apache/nifi/commit/e36b37692c528b2e9774f9f49737eb2d1a05960f#diff-7f99fec326074ddcb38d63156204fbcff6693bf7dc5eedd60c6a477af00d608b)
There is a discussion that touches on it [here](https://github.com/apache/nifi/pull/319#discussion_r59995999)
but it is unclear why this functionality was removed for stateless.
My best guess is this was an oversight that slipped in due to the many other things happening in that commit.
I just tested it before and after the fix and in both cases the flowfiles will accumulate on input queue and age off (if age off is set).
Also, if flow containing UpdateAttribute with malformed EL is in registry, it can be put under version control before the fix and nifi can be updated with fix and/or a new instance of flow can be pulled to nifi (after the fix) without issues (i.e. there will be a green checkbox on the process group in both these cases).
So the sanity checks pass as far as existing users seeing new behavior that could cause them concern so we should be good, but hopefully @markap14 can take a quick look to see if something is being overlooked here.
In any event, thank you once again @mattyb149. I greatly appreciate you taking the time to look at this!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 closed pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 closed pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
URL: https://github.com/apache/nifi/pull/6770
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by "NissimShiman (via GitHub)" <gi...@apache.org>.
NissimShiman commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1404440364
Thank you @mattyb149 and @joewitt !
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1379417107
+1 LGTM but would like to get @markap14 's thoughts on this, I assume there was a reason why we didn't have an EL validator for non-stateful mode. Having said that, if an existing UpdateAttribute becomes invalid when upgrading to a NiFi that has this PR, then it appears bound to fail anyway, so marking the processor as invalid seems legit.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] joewitt commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1403885588
Wanted to consider for 1.20 but dont want to block for it. @mattyb149 can you please look at this in the context of what would happen if the EL for a property returns a boolean but this code now implies the return value must be a string? Woudl it then be invalid?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1358016432
Thank you @markobean for looking at this (and other PRs I have submitted in the past)!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by "mattyb149 (via GitHub)" <gi...@apache.org>.
mattyb149 commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1403909954
+1 LGTM, tested expressions that return different types (boolean, integer) and all were validated as expected. Thanks for the improvement! Merging to main
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1357814683
Nice catch and fix. I'm surprised this wasn't noted and fixed a long time ago since this processor is so heavily used.
I confirmed bad behavior in 1.19.1 when using state in UpdateAttribute; reviewed the code, installed the fix and verified the processor remains invalid when bad EL is used - regardless of state usage.
Thanks @NissimShiman!
+1
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6770: NIFI-10853 Allow UpdateAttribute dynamic property to validate nifi expressions
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6770:
URL: https://github.com/apache/nifi/pull/6770#issuecomment-1397446624
@markap14 Could you look at this and comment about whether there would be an issue if UpdateAttribute processor had an EL validator for non-stateful mode.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org