Skip to content
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

incomplete: Fix hessian2 serialized short, byte is converted to int bug (#1232) #1609

Closed
takeseem opened this issue Apr 15, 2018 · 7 comments

Comments

@takeseem
Copy link
Contributor

takeseem commented Apr 15, 2018

#1232
It is just supported Hessian2, imcomplete in hessian1.

Example: https://github.com/apache/incubator-dubbo/blob/3d4f2fcdbbc327c2dac872962f65b2f011c7d247/hessian-lite/src/test/java/com/alibaba/com/caucho/hessian/io/Hessian2StringShortTest.java#L47

if add hessian1 test, it will be fail:

    @Test
    public void serialize_string_byte_map_then_deserialize() throws Exception {

        Hessian2StringShortType stringShort = new Hessian2StringShortType();
        Map<String, Byte> stringByteMap = new HashMap<String, Byte>();
        stringByteMap.put("first", (byte)0);
        stringByteMap.put("last", (byte)60);
        stringShort.stringByteMap = stringByteMap;

        Hessian2StringShortType deserialize = baseHession2Serialize(stringShort);
        assertTrue(deserialize.stringByteMap != null);
        assertTrue(deserialize.stringByteMap.size() == 2);
        assertTrue(deserialize.stringByteMap.get("last") instanceof Byte);
        assertEquals(Byte.valueOf((byte)0), deserialize.stringByteMap.get("first"));
        assertEquals(Byte.valueOf((byte) 60), deserialize.stringByteMap.get("last"));

//add hessian1 test, it will be fail
        deserialize = baseHessionSerialize(stringShort);
        assertTrue(deserialize.stringByteMap != null);
        assertTrue(deserialize.stringByteMap.size() == 2);
        assertTrue(deserialize.stringByteMap.get("last") instanceof Byte);
        assertEquals(Byte.valueOf((byte)0), deserialize.stringByteMap.get("first"));
        assertEquals(Byte.valueOf((byte) 60), deserialize.stringByteMap.get("last"));
    }

so my fix push test fail: #1608 JavaDeserializer._constructor.newInstance([null, null]) NPE
old issue #210 dubbo调用报错HessianFieldException

@zonghaishang
Copy link
Member

zonghaishang commented Apr 16, 2018

The hessian2 protocol has been used by default. Which scenario needs to use hessian1? Please tell me
which version you are using.

@takeseem
Copy link
Contributor Author

takeseem commented Apr 16, 2018

dubbo作为一个严谨的框架,既然hessian1未废弃,就应当保持一致性,因为其他人可能在使用。

这是我第3次看到dubbo的master,合并到主干时未考虑兼容性。

建议先完成:功能性审核,再讨论代码样式和规范。

@zonghaishang
Copy link
Member

Dubbo does not use the hessain1 protocol.
I have fix hessian1 protocol , see: #1616

@takeseem
Copy link
Contributor Author

takeseem commented Apr 18, 2018

今天仔细看下代码,hessian1确实没有用了,建议从hessian-lite中移除。
多余的代码需要维护!

这确实是我对dubbo了解的还不够,想当然看到有hessian1就以为还需要兼容。

@chickenlj
Copy link
Contributor

Hi takeseem,

Thanks for your suggestion, also very sorry for the incompatible problems brings to you. We should pay more attention on change compatibilities, especially for legacy systems using old framework/protocol versions. At least, there can be a compatible notification.

@chickenlj
Copy link
Contributor

For hessian1, i also recommend remove from hessian-lite

@diecui1202
Copy link

#1616 has been merged. I submit a new issue for hessian-lite #1778

So please close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants