Don't require workspace for reading from text parameter. #31
Conversation
steveprentice
commented
Sep 6, 2017
steveprentice
commented
- Created abstract base classes for read steps to remove duplication.
- Updated tests to not allocate node while reading text.
* Created abstract base classes for read steps to remove duplication. * Updated tests to not allocate node while reading text.
|
|
||
| @Override | ||
| public AbstractFileOrTextStep newInstance(Map<String, Object> arguments) throws Exception { | ||
| AbstractFileOrTextStep step = DescribableHelper.instantiate(stepType, arguments); |
There was a problem hiding this comment.
super.newInstance should suffice
There was a problem hiding this comment.
Sure does. For some reason, I thought that super.newInstance would return an instance of Step, but I see now that it gets the enclosing class and uses that. Updated to use super, which means we can remove the stepType field entirely.
| private String text; | ||
| private Map<String, Object> defaults; | ||
| public class ReadPropertiesStep extends AbstractFileOrTextStep { | ||
| private Map defaults; |
There was a problem hiding this comment.
Sorry. Should have added a note here on why this was changed. Two reasons:
- The primary reason is because of type erasure. All we know about the defaults Map at runtime when it is passed into newInstance as an entry in the arguments Map is that it is an
instanceof Map. Taking that an assigning it to this parameterized type is an unchecked assignment. - Because of a bug/limitation of
DescribableHelper.instantiate(), it'll throw a ClassCastException whenever it tries to build and coerce arguments for a method with any parameterized args. The offending code in workflow-step-api-plugin makes an incorrect assumption that it can always cast aTypetype to aClass. Changing this back toMap<String, Object>will trigger this bug. The reason this wasn't seen before is that this class was previously rolling its ownnewInstance()(possibly to work around this bug).
Fixing 2 would allow us to use super.newInstance() with a parameterized type, but because of 1, I don't think it is desirable.
There was a problem hiding this comment.
OK, have you filed a bug report about that?
There was a problem hiding this comment.
@rsandell, I looked into this a bit more and it looks like it has been fixed upstream a long while ago, but the dependencies declared in this plugin use very old versions of workflow-step-api. As of version 2.0 of workflow-step-api, newInstance() now uses the structs plugin's DescribableModel class instead, which appears to have a fix for this issue since early 2016.
Is there a reason the pom.xml of this plugin still points to old versions of the workflow-step-api? If not, I can open a PR to bump them.
(Also, thanks for merging and releasing!)
There was a problem hiding this comment.
Pure laziness and lack of time. There is more work to do than just bump the dependencies;
- The pom needs to be adapted to plugin parent 2.x
- then bump the dependencies at the same time, inclusing core to something like 2.7.3 at least
- All the
@Injectand@StepContextParameterset.al. needs to be changed and adapted to the new way of doing things.
Most of those things are not strictly needed, but I prefer this plugin to be as "pure" as possible and that desire made it just a bit too overwhelming for me to find time to do it.
If you're up for it you are welcome to.
|
|
||
| /** | ||
| * Execution of {@link ReadPropertiesStep}. | ||
| * | ||
| * @author Robert Sandell <[email protected]>. | ||
| */ | ||
| public class ReadPropertiesStepExecution extends AbstractSynchronousNonBlockingStepExecution<Map<String, Object>> { | ||
| public class ReadPropertiesStepExecution extends AbstractFileOrTextStepExecution<Map<Object, Object>> { |
There was a problem hiding this comment.
This boils down to type erasure again since all we know about defaults is that it is a Map, when we do result.putAll(step.getDefaults()), we are performing an unchecked assignment. Additionally, the Properties class itself is of type Map<Object, Object>, so it made since to remove the unchecked assignments.
Having said that, I changed this back to Map<String, Object> and perform a type safe reassignment instead of using putAll().
| @@ -165,6 +95,8 @@ public Step newInstance(Map<String, Object> arguments) throws Exception { | |||
| */ | |||
| @Override | |||
| protected Object run() throws Exception { | |||
| super.run(); | |||
There was a problem hiding this comment.
Another, perhaps better approach is to have another abstract method in AbstractFileOrTextStepExecution that is called from it's run method and then override that abstract method here. That way you don't forget to call super.run(); in every sub class.
There was a problem hiding this comment.
Updated. Added an abstract doRun() and made run() final.
| public AbstractFileOrTextStep newInstance(Map<String, Object> arguments) throws Exception { | ||
| AbstractFileOrTextStep step = DescribableHelper.instantiate(stepType, arguments); | ||
| if (isBlank(step.getFile()) && isBlank(step.getText())) { | ||
| throw new IllegalArgumentException("At least one of file or text needs to be provided."); |
There was a problem hiding this comment.
Even though it has no real function, having all messages listed in Messages.properties would be good.
Changes based on code review: * Use super.newInstance in AbstractFileOrTextStepDescriptorImpl * Extract localizable string in Messages.properties. * Revert changed return type. * Make run() final and add abstract doRun() method.
|
I reviewed the failed validation build as well, and it doesn't look related. The windows branch passed and the linux branch timed out. Perhaps it can be kicked off again? |
|
Just trying to tickle the PR builder |
