[v7r2] Fix JSON encoding of RMS requests

[chrisburr]

It turns out #5091 isn't the end of the story of encoding issues in the RMS. The LHCbDIRAC unit tests showed that the toJSON methods are broken due to bytes being an invalid type in JSON. I don't like it but 3ce9622 seems to be a good enough solution with the view to eventually removing the use of DEncode.

As part of this I modernised the tests to use pytest, it might be easier to review if you ignore the first commit. While doing this I noticed that many of the tests of the form:

  try:
    theFile.OperationID = 111111
  except Exception as error:
    assert isinstance(error, AttributeError)
    assert str(error) == "can't set attribute"

are actually broken as the expected exception isn't raised. I've commented them out in 1c2b528 while we decide what to do about them.

BEGINRELEASENOTES

*RequestManagement
FIX: Fix JSON encoding of RMS requests

ENDRELEASENOTES

[andresailer]

Use assertRaises https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises to notice when tests are broken?

[chrisburr]

I modified the tests to use pytest.raises in 2f6ebfe which is what made me notice.

[chaen]

As we discussed, these tests should just be removed, they do not make sense since we removed this type checking logic in 2014.

As for the json stuff. I am not convinced this is the correct thing to do. I suspect we are going to run into this issue with every DB that has BLOB types no ? I am wondering whether we are not better off having such a fix in JEncode and use JEncode instead of the specific code RMSEncoder (actually, the RMSEncoder is the ancestor of JEncode :-D )

[chaen]

Actually, thinking of it again.... it sort of boils down to the same issue as with the CS, so should we apply the same trick, that is base64 ?

[chaen]

we could do it in JEncode in a similar way than we do that ?

DIRAC/src/DIRAC/Core/Utilities/JEncode.py

Lines 100 to 101 in 54d8988

	if isinstance(obj, datetime.datetime):
	return {'__dCls': 'dt', 'obj': obj.strftime(DATETIME_DEFAULT_FORMAT)}

Do you see any downside to it ?

[chrisburr]

Do you see any downside to it ?

I think I agree with you, specifically:

• SourceComponent should be converted to VARCHAR instead of BLOB
• JEncode should be used instead of DEncode for making Arguments serialisable
• The Arguments can then also be converted to VARCHAR instead of BLOB

My only question would be how we deal with the actual upgrade/migration and I suspect it's invasive enough that it is something for v7r3.

[chaen]

Converting BLOB to VARCHAR for SourceComponent should be a no brainer from the DB point of view, since it is all string. For Arguments, up to now in DIRAC it has only been text serialized with DEncode so I guess that works too.

We should indeed get ride of the RMSEncoder and favour JEncode for the network serialization of the overall RMS objects, but the Argument field should stay in the DB with DEncode, since it will be tricky to migrate that. The problem mainly is that the encoding/decoding is not done in the Operation class, but scattered in the client code, which is a bit stupid. So if we want to change that (but do we ?), a first step is probably to factorize the encoding/decoding of Arguments into the Operation class.
(we had a general discussion about that here #4986)

Of course, the problem will always be the compatibility of old clients. Though for that we could finally make use of the clientVersion field sent in the dips connection arguments...

I somehow have the feeling that whatever modification we want to do with BLOB (in any DB), we are better of doing it BEFORE running actual python3 code

[fstagni]

I think the discussions here should be converted into a issue, and at the same time merge this PR, because as it is, for existing installations, and for v7r2, it looks just fine.
For v7r3 we should at least find a way to address the issues above.

[chrisburr]

Done, if there are no further comments shall we get this merged so we can unblock the LHCbDIRAC CI?

[chaen]

yep !