sbcgua / ajson

Yet another json parser serializer for ABAP
MIT License
50 stars 15 forks source link

Fix: assertion failure caused by include structures with suffixes #168

Closed mathieulawnik closed 1 year ago

mathieulawnik commented 1 year ago

In recursive calls of convert_struc() on suffixed include structures, the data holds the entire structure that the include structure is part of, so components of the include are suffixed, however the type is determined from the include structure alone, so the component list holds component names without their suffix. This leads to an ASSERTION_FAILURE error after component assignment fails because the names do not match.

mathieulawnik commented 1 year ago

See: https://github.com/abapGit/abapGit/issues/6386

sbcgua commented 1 year ago

Thanks for the contribution. Could you also please add a unit test covering the issue.

mathieulawnik commented 1 year ago

Removing suffixes wasn't right because it changed first-level components' names. I've replaced the call to method get_components() with a call to get_included_view(). The structures are now converted flat instead of recursively.

mathieulawnik commented 1 year ago

I assume the workflow test fail because the method cl_abap_structdescr=>get_included_view() does not exist in 702.

mbtools commented 1 year ago

The method is not implemented in open-abap which is used to run UTs. https://github.com/open-abap/open-abap-core/blob/main/src/rtti/cl_abap_structdescr.clas.abap

You could try to add it there or work with @larshp to get it implemented

sbcgua commented 1 year ago

The code change looks good from my perspective. So can be merged as soon as the CI stuff passes.

larshp commented 1 year ago

I'll fix the issue in open-abap, just need some time...

larshp commented 1 year ago

its kind of a mess for me to fix, so it will be a while, suggest adding a skip to https://github.com/sbcgua/ajson/blob/master/transpile_for_testing.json#L22

mathieulawnik commented 1 year ago

An alternative to using get_included_view() would be to pass the prefix to the convert_struc() method and append it to the node names.

larshp commented 1 year ago

keep the code IMHO, avoid workarounds due to the open-abap runtime

sbcgua commented 1 year ago

its kind of a mess for me to fix, so it will be a while, suggest adding a skip to https://github.com/sbcgua/ajson/blob/master/transpile_for_testing.json#L22

:+1:, ok for me

sbcgua commented 1 year ago

Merging. I'll add the test exception. @mathieulawnik Thanks for the contribution!

sbcgua commented 1 year ago

@larshp Did you fixed the issue on the transpiler side? Because the tests pass now. (If yes, thank you! =)

larshp commented 1 year ago

hmm, no, I think its fixed in https://github.com/open-abap/open-abap-core/pull/737/commits/72b6a4d57bbce6ac121b320be585e7897f79ee71, but its not merged yet

mbtools commented 1 year ago

looks like the fix was in https://github.com/open-abap/open-abap-core/pull/737