Jsr223 fixes and updates #3576
Jsr223 fixes and updates #3576
Conversation
| } | ||
| throw new IllegalArgumentException("No enum constant " | ||
| + UnDefType.class.getCanonicalName() + "." + name); | ||
| } |
There was a problem hiding this comment.
Why not valueOf instead of parse so you don't need a special case in TypeParser? What do you mean in the comment "valueOf does not work because of the toString method"?
Also, what is the likely impact of this core change? Could you separate out the UnDefType-related changes since they are wider in scope than Jsr223?
There was a problem hiding this comment.
I can't override the valueOf method in an enum:
The enum UnDefType already defines the method valueOf(String) implicitly
Try it:
UnDefType.valueOf("NULL"): Uninitialized
UnDefType.valueOf("UNDEF"): Undefined
UnDefType.valueOf(UnDefType.NULL.toString()): IllegalArgumentException: No enum constant org.openhab.core.types.UnDefType.Uninitialized
UnDefType.valueOf(UnDefType.UNDEF.toString()): IllegalArgumentException: No enum constant org.openhab.core.types.UnDefType.Undefined
The TypeParser never worked with the UnDefType
I found this problem while i wrote a jsr223 script, i wanted to update a item:
postUpdate("ITEM_NAME", UnDefType.UNDEF);
i got the IllegalArgumentException and searched the reason
There was a problem hiding this comment.
Yes, this is a case where a core type's toString() result is not acceptable to the class's valueOf static method. But I'm concerned that there would be wide ripple effects from this change, as some item classes list UnDefType ahead of other types in the class's acceptedDataType list, so other code elsewhere in openHAB could experience a breaking change. Could a code change to fix the call from a Jsr223 script to
postUpdate("ITEM_NAME", UnDefType.UNDEF);
happen in the Jsr223 code? Is the script language just unable to reference Java enums using that syntax?
There was a problem hiding this comment.
This is a general bug, i am surprised that this did not pop up earlier. I can implement a workaround especially for jsr223, but i think the TypeParser is the correct place and fixes it globally.
It only affects the UnDefType class, so there should not be much side effects i think. My installation runs fine without any problems. Maybe we should ask @kaikreuzer or @teichsta ?
There was a problem hiding this comment.
Maybe we should ask @kaikreuzer or @teichsta ?
👍
There was a problem hiding this comment.
Yes i know, postUpdate(ITEM, UnDefType.UNDEF) works as expected.
Note that you can't just do UnDefType.UNDEF.toString() for the second argument because depending on the item type it might match another acceptable type. StringItem is one example where that would happen.
The TypeParser uses foreach for the getAcceptedDataTypes list. foreach uses the iterator from the ArrayList which returns the entries in proper sequence. In the StringItem, StringType is the first acceptedDataType which always match first.
In each Item, the UnDefType is the last acceptedDataType, except the RollershutterItem. But this item works too, because it's values never matches the string NULL or Undefined.
There was a problem hiding this comment.
OK, I suppose I'm not clear then about the JSR223 problem being solved with this change.
There was a problem hiding this comment.
It is solved, i am currently writing some JSR223 scripts and with this changes it works as expected for some days now without any issues so far.
There was a problem hiding this comment.
That's great, but the specific problem the change is intended to fix is still not clear to me. I have many JSR223/Jython scripts and they work as expected even without this change. I agree with @watou that this should be broken out into a separate PR.
There was a problem hiding this comment.
I am using JavaScript an run into this issue. OK, then close this PR and i can create two separate
sorry for the late reply. In ESH the UndefType issue has been solved by simply removing the toString() method which i suggest to downport to OH1 as well (see #3679). |
This is imho a very bad idea. In ESH I did that, knowingly breaking backward compatibility. For openHAB 1, this now means that existing rules and other code might not be compatible with 1.7 anymore, which will probably cause problems for many users. |
|
ok, reverted this PR #3690. Nevertheless i scanned through the code and it seems that codewise we were not going to expect any problems. For rules you will be right for sure … |
No description provided.