-
Notifications
You must be signed in to change notification settings - Fork 407
Fix #5164 #5647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix #5164 #5647
Conversation
Shouldn't this be fixed on the formula side? |
What about backward compatibility? It breaks any pipeline using formula transform. That's the reason why it is better IMHO to manage it this way and do the things right by fixing the formula transform (and all other transforms with same issue) in a later major release. But I'm open to things in another way. |
The bug reads like "it never worked" so do we have a backwards compatibility problem? |
The problem is related to the fact that Formula transform always wrote value_type attribute as a code instead of the name of the value type (ex.: 2 instead of String for example). We are checking if this is the case for any other transform that require the value_typoe attribute saved in its metadata set. Because of this, in the Metadata Injection dialog, the user is required to insert for the value_type attribute the value of 2 (the code of the value type) instead of for example of String (the name of the value type) to have the Metadata Injection transform working. That is a bit weird for the user. My fix makes possible for the user to write the name of the value type instead of its code in the Metadata Injection dialog. Fixing this issue this way is not the best option I know. It's a sort of workaround. That should be fixed in the formula transform so that it saves the things in the correct way. But doing this that way breaks everything because risks not to be backward compatible. I can provide a sample, that is the same sample I used for the tests, to show you that "it never worked". I will attach it to the issue. But the important point here is the way we fix it. IMHO, my solution is a good compromise before the next major release where we will fix it the way it should be in the correct way. |
// target transform has specified key | ||
String value = variables.resolve(source.getField()); | ||
injector.setProperty(targetTransformMeta, target.getAttributeKey(), null, value); | ||
if (targetTransformMeta.getClass().getSimpleName().equals("FormulaMeta") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really can't wrap my head around this. Why is this needed @sramazzina ?
Fix #5164 - value type in "Formula" transform injection
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean install apache-rat:check
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i
.addresses #123
), if applicable.To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.