[#262] Fix Action model value type #263
Conversation
Dohbedoh
force-pushed
the
bugfix/actionscomplexvalues
branch
from
1d579d4 to
c057362
Compare
|
|
||
| @Test | ||
| public void testTriggerWithComplexActionValues() throws Exception { | ||
| String json = "{\"id\":12345678,\"title\":\"Test Trigger\",\"active\":true,\"position\":45,\"conditions\":{\"all\":[],\"any\":[{\"field\":\"comment_includes_word\",\"operator\":\"includes\",\"value\":\"@triggerTest\"}]},\"actions\":[{\"field\":\"notification_target\",\"value\":[\"1234567890\",[[\"key1\",\"value1\"],[\"key2\",\"value2\"]]]}],\"createdAt\":null,\"updatedAt\":null}"; |
There was a problem hiding this comment.
@Dohbedoh Maybe it's like this in others tests but couldn't we move them to resources files and load them to have a better readability ?
There was a problem hiding this comment.
Done. Note: the compact json is used for the last assertion of each test.
| } | ||
|
|
||
| public void setValue(String[] value) { | ||
| public void setValue(Object[] value) { |
There was a problem hiding this comment.
The internal structure change is OK.
The problem, I believe, is changing the method signatures. The clients compiled against the older versions won't be able to use this new version because of class casting or method-binding issues if I understand correctly.
This, therefore, might need a major version bump.
There was a problem hiding this comment.
In the past I used Clirr ( http://www.mojohaus.org/clirr-maven-plugin/ ) but I don't think it was updated to manage recent java versions. But for sure such tools are existing @duemir @Dohbedoh
There was a problem hiding this comment.
Right I think this breaks compatibility. Although the signature change to a broader class... I am not sure at the moment how we could avoid this to fix that bug. Only solution I could think of is to use a custom Jackson serializer.
There was a problem hiding this comment.
Arrays are covariant in Java so the setter is probably not an issue.
The getter is what worries me since in the client's code there'll be an assignment to a String[] but the new code returns Object[]
There was a problem hiding this comment.
I remember reading a solution for this kind of issue used by netbeans consisting of patching the bytecode.
Here you go: https://books.google.fr/books?id=VXKBgiD57lwC&printsec=frontcover&source=gbs_ge_summary_r&cad=0#v=onepage&q&f=false
Go page 44, google is kind enough to grant access to this specific page :)
Read the Mangling the Class file content section (read the full book if you have the opportunity it's rich :)).
Dohbedoh
force-pushed
the
bugfix/actionscomplexvalues
branch
from
b84f846 to
042b5bd
Compare
4 participants
@reviewbybees