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

Error while JSON::PP::Boolean key exists in incoming XML mapping #3765

Closed
romanpilipovskiy opened this issue Sep 5, 2024 · 14 comments
Closed
Assignees
Labels
bug Something isn't working as intended
Milestone

Comments

@romanpilipovskiy
Copy link

When trying to integrate OTOBO with GitLab, I get an error when processing the response message, since it contains fields with a value 'has_tasks' => bless( do{\(my $o = 1)}, 'JSON::PP::Boolean' ) or 'locked' => bless( do{\(my $o = 0)}, 'JSON::PP::Boolean' ),

Errors:
Can't encode a value of type: JSON::PP::Boolean at /opt/otobo/Kernel/GenericInterface/Mapping/XSLT.pm
Could not convert data from Perl to XML before mapping

@bschmalhofer bschmalhofer added the bug Something isn't working as intended label Sep 5, 2024
@bschmalhofer
Copy link
Contributor

Hi Roman,

this looks indeed like a bug in OTOBO. This occurs when the JSON returned from Gitlab is converted to XML. The reference to JSON::PP::Boolean are to be expected in the parsed JSON. They represent boolean values. But XML::Simple does not know how to handle these references. See

$XMLSimple->XMLout(
. We will take a look at this issue.

@romanpilipovskiy , which version of OTOBO are you running?

@romanpilipovskiy
Copy link
Author

romanpilipovskiy commented Sep 5, 2024

@bschmalhofer 11.0.5

@StefanRother-OTOBO
Copy link
Contributor

There is a bug in JSON::XS, I think we have to use Cpanel::JSON::XS in the long term. It works with that.

Workaround:
Install Cpanel::JSON::XS, if not already present.
Then copy the file Kernel/System/JSON.pm to Custom/Kernel/System/JSON.pm

Then adjust the file Custom/Kernel/System/JSON.pm and replace all occurrences of JSON::XS with Cpanel::JSON::XS.

Insert the following line below the line ‘$JSONObject->allow_nonref(1);’ in the ‘sub Decode’:

$JSONObject->unblessed_bool([1]);

@bschmalhofer: I think we need to switch to Cpanel::JSON::XS at least for 11.1, or what do you think?

@StefanRother-OTOBO StefanRother-OTOBO added this to the OTOBO 11.1 milestone Nov 6, 2024
@bschmalhofer
Copy link
Contributor

I think there was a reason for not using Cpanel::JSON::XS, even though it did appeal to me initially. I'll try to find that reason.

Instead of using unblessed_bool() from Cpanel::JSON::XS we can also use boolean_values() from JSON::XS.

@bschmalhofer
Copy link
Contributor

bschmalhofer commented Nov 7, 2024

The problem with Cpanel::JSON::XS seemed to be an installation issue. A version was required that was available on CPAN but not in the OS repositories. See 7a87c0c . So I still prefer to stick with JSON::XS and not switching back and forth.

I see that

$XMLSimple->XMLout(
is calling XML::Simple::XMLOut() directly. Switching to using the module Kernel::System::XML::Simple, which is using XML::LibXML::Simple internally, is not an option. This is because XML::LibXML::Simple only provides XML In(). So I propose to use https://metacpan.org/pod/XML::Simple#Handler-=%3E-object_ref-#-out-SAX-only to generate real booleans in the generated XML. I think that this is the most intuitive option. Boolean values in JSON should also be used as Boolean XML values in the XSLT transformation.

TODO:

  • provide a test case with boolean values in the incoming JSON
  • Make XML::Simple::XMLOut() work with blessed objects in the parsed JSON
  • Think about whether the fix can be applied to older OTOBO releases, in patch level versions

@StefanRother-OTOBO
Copy link
Contributor

Hi Bernhard,

ok thanks, that's fine for me. We have access to a test case and I'm able to share it.
However, I don't understand how it works with SAX right now, but I'm not a developer either.
If you need access to the test case, please feel free to contact me.

Thanks!
Stefan

@bschmalhofer bschmalhofer self-assigned this Nov 12, 2024
bschmalhofer added a commit that referenced this issue Nov 12, 2024
use subtests for grouping related tests
@bschmalhofer
Copy link
Contributor

I checked how the XSLT mapping parsed the XML that was generated from the passed data. No schema information is given for the XML source. This means that XSLT templates need to match on the strings 'true' and 'false'. This is fine.

@svenoe
Copy link
Contributor

svenoe commented Nov 13, 2024

I checked how the XSLT mapping parsed the XML that was generated from the passed data. No schema information is given for the XML source. This means that XSLT templates need to match on the strings 'true' and 'false'. This is fine.

For my understanding: Is this not making the whole "having a real boolean in the XML", which I understand is the aim of using SAX, superfluous? If in XSLT we are checking for strings anyways, then let's not do all the hassle and rather just use boolean_values() from JSON::XS to directly put those strings in the XML. (The latter did from a quick test not work for us, just now, contrary its Cpanel counterpart, but that is probably on us.)

@bschmalhofer
Copy link
Contributor

I would still prefer to do it the more "correct" way. At the end of the day, adding the Handler() is not much more effort than calling boolean_values().

@svenoe
Copy link
Contributor

svenoe commented Nov 14, 2024

Passt ;) - the pod of the SAX packages just looked a lot more involved than the one method of either JSON::XS implementation. So if it doesn't come with a whole new set of dependencies, significantly more code, and/or computational effort, I'm fine with either solution.

@bschmalhofer
Copy link
Contributor

I did some further digging as the cause and thehistory of this bug is a bit convoluted.

The OTOBO releases 10.0 and 10.1 were using the module JSON. Effectively this was either JSON::PP or JSON::XS. Both of these modules decode JSON booleans into instances of JSON::PP::Boolean. Note that JSON::PP::Boolean can also be an alias of Types::Serialiser::Boolean. In order to avoid the blessed variables, the decoded data structure was tree walked in the sub _BooleansProcess(). This tree walker replaced the blessed variables with unblessed 1 or 0.

The bug was introduced in #399 where Kernel::System::JSON was based in Cpanel::JSON::XS. For that issue the tree walking support in _BooleansProcess() was removed because it was deemed to be no longer needed. The usage of Cpanel::JSON::XS was reverted to JSON::XS while working on #2595. But the removal of _BooleansProcess() was not rolled back. Actually the choice between Cpanel::JSON::XS and JSON::XS is not relevant here, as both behave the same way.

The question remains why the significant change was not detected by the unit tests. This has to do with the peculiarities of Types::Serialiser::Boolean, which overloads numification. The comparison method is seems to check numeric equality and does not bother the check the class.

So the fix for OTOBO 11.0.x is to not emit blessed references, which confuse XML::Simple. For JSON::XS this can be done by calling boolean_values(). Cpanel::JSON::XS would have to call unblessed_booleans()` instead.

My previous idea was to tweak the behavior of XML::Simple by using the method Handler(). It turns out that this is not feasible. This is because first the XML is generated as a string, and then the SAX events are emitted by parsing the generated string. So XMLout() fails before the SAX events are being generated.

bschmalhofer added a commit that referenced this issue Nov 14, 2024
use subtests for grouping related tests
bschmalhofer added a commit that referenced this issue Nov 15, 2024
more precisely, with test input that is the result of parsing JSON
bschmalhofer added a commit that referenced this issue Nov 15, 2024
use subtests for grouping related tests
bschmalhofer added a commit that referenced this issue Nov 15, 2024
more precisely, with test input that is the result of parsing JSON
@bschmalhofer
Copy link
Contributor

Hi @romanpilipovskiy ,

sorry, this looks like a bug that was introduced in OTOBO 11.0.1. The behavior had changed and unfortunately the test suite did not catch the change. I have created a fix that restores the behavior from OTOBO 10.1.x. The JSON boolean values true and false appear now again as 1 and 0 in the intermediate XML file.

The fix will be included in the next patch release. In the meantime you could apply the hotfix to your installation,

git show 2146528b3426727750958494353e91ea071965ed Kernel/System/JSON.pm
commit 2146528b3426727750958494353e91ea071965ed
Author: bernhard <Bernhard.Schmalhofer@gmx.de>
Date:   Thu Nov 14 15:03:06 2024 +0100

    Issue #3765: emit plain 0 and 1 for JSON boolean values

diff --git a/Kernel/System/JSON.pm b/Kernel/System/JSON.pm
index af6355f845..7c069e0da3 100644
--- a/Kernel/System/JSON.pm
+++ b/Kernel/System/JSON.pm
@@ -185,6 +185,15 @@ sub Decode {
     # grudgingly accept data that is neither a hash- nor an array reference
     $JSONObject->allow_nonref(1);
 
+    # In OTOBO 10.0.x and OTOBO 10.1.x there is a tree walker that
+    # replaces the boolean values, that is instances of JSON::PP::Boolean,
+    # with the plain integer values 0 and 1.
+    # This behavior is reproduced with explicitly declaring
+    # what should be emitted for JSON booleans 'true' and 'false'.
+    # Note that when using Cpanel::JSON::XS, the attribute unblessed_bool can be used
+    # for the same purpose.
+    $JSONObject->boolean_values( 0, 1 );
+
     # Deserialize JSON and get a Perl data structure.
     # Use Try::Tiny as JSON::XS->decode() dies when providing a malformed JSON string.
     # In that case we want to return an empty list.



@romanpilipovskiy
Copy link
Author

@bschmalhofer Thank you!

@bschmalhofer bschmalhofer modified the milestones: OTOBO 11.1, OTOBO 11.1.1 Dec 7, 2024
@bschmalhofer
Copy link
Contributor

A similar issue was reported for OTOBO 11.0.x in the OTOBO forum https://otobo.io/forums/topic/json-issues-after-migrating-to-otobo-11/ .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended
Projects
None yet
Development

No branches or pull requests

4 participants