Stop parsing tickets as HJSON

[floriannari]

We noticed that Apereo CAS spent a lot of time deserializing HJSON into JSON

Indeed, whenever we want to read a JSON, AbstractJacksonBackedStringSerializer calls org.hjson.JsonValue.readHjson(String).toString() to convert the String from HJSON to JSON.
Unfortunately this is very costly in terms of performance, whereas in most cases the String is already a JSON string.

This commit aims to stop converting from HJSON to JSON for Strings that may not have been written in HJSON (tickets, for example).

[apereocas-bot]

Thank you very much for submitting this pull request!

What happened?

The patch presented here seems to contain no adequate unit and/or integration tests. Patches that alter CAS core functionality without proper tests, regardless of how small they may seem, are automatically closed. By tests, we specifically mean, unit/integration/browser/functional tests; a description and series of instructions on how one may go about reproducing or testing the patch manually is excellent but not sufficient for acceptance. If the change-set here modifies CAS functionality that is extremely and obviously trivial (i.e. fixes a typo) or updates the CAS documentation or presents any other type of change that generally can be accepted without proper tests, please comment and explain on the pull request.

What's next?

Please re-open the pull request (or ask project maintainers to do this for you) when you're ready and have included tests to verify functionality. If you feel like you are not immediately available or have no time to work out test cases and steps to reproduce and verify the changes, that's not an issue. Please revisit the pull request as soon as it's convenient for you to resume work as your schedule allows. The pull request will wait for you right here.

If you'd like to keep the patch around and open while you make progress, please mark the patch as WIP using one of the following methods:

• Construct your pull request title using the syntax WIP - <title>
• Create the initial pull request in "Draft" status.

If you do not know how to write tests, please browse through the resources listed below and get started. If you get stuck, please feel free to ask for help.

Resources

Furthermore,

• To learn about CAS test process and how you may design and develop unit/integration tests, please see this link. If you are unclear on how to write basic unit tests, please see this link.
• See Contributor Guidelines to learn more about how to contribute to the project and review the criteria that allows for the project to accept contributions.
• If you are seeking assistance and have a question about your CAS deployment, please visit this page to learn more about support options.
• If your change-set attempts to solve a problem or defect, be sure to include tests to first and foremost demonstrate the issue. Tests must be able to reproduce the problem without changes and fixes first so that project maintainers can ascertain the validity of the issue scenario without resorting to manual steps. Tests and testing done after the fact is not acceptable.
• If your change-set introduces a new CAS feature or improvement, please consider starting discussion with the project to see if your patch can be accepted and maintained by project owners and maintainers. Unless your change is extremely trivial and has been through proper review and is sponsored by a project maintainer, please do not spend time modifying code and configuration in a proposed pull request.

If you believe this message to be an error, please post your explanation here as a comment and it will be reviewed.

Support

If you are seeking assistance or have a question about your CAS deployment, please visit support options to learn more.

Thanks again!

[floriannari]

I don't think there's a need to add any additional tests?

[floriannari]

PS: I also backposted this commit in 7.0 and 7.1

[hdeadman]

I suggest finding something to write a test for, to get past the bot that will otherwise close the PR. If nothing else, maybe a unit test that shows the difference in time between parsing a ticket with and without the hson -> json conversion.

[floriannari]

@mmoayyed What do you think?
Do I absolutely have to add new (possibly unnecessary) tests? (If so, which tests in particular would you like ?)
Or are the existing tests already in Apereo CAS sufficient?

[floriannari]

In practice, with this modification, we're seeing a drop of around 40% in CPU usage on our CAS server. (Keeping in mind that the server also hosts the MongoDB used by CAS)

[hdeadman]

How about a test that parses a ticket a hundred times as hson and json to demonstrate the speed difference? Even if the test gets deleted eventually, it would serve the purpose of getting your PR to stay open and reviewed.

[mmoayyed]

Thank you for the find and the change.

The PR here cannot be accepted. The approach taken with extending base classes is not maintainable and cannot scale. You can imagine that if we add yet another variation of the parser or a change in settings, you would have a cartesian product of all implementations.

Your changeset is also unnecessary for the master branch. Review what is done already as an example, and you're welcome to apply the same logic to other branches in maintenance, with small changes.

Unless you're fixing a typo or changing a background color, tests are always a must. To verify the behavior, to show that you have actually tested this, and to communicate how the change works and what impact it makes. This is why the bot exists :) A screenshot of "it works better" is never acceptable, verifiable or reason to keep the change around intentionally or accidentally.

Also, please don't write tests to make the bot happy. Write tests to test.

[prigaux]

Seems to be fixed in master in commit 2445f63 (together with some fixes specific to redis ticket registry)

=> now only RegisteredServiceJsonSerializer is doing HJSON

[floriannari]

New PRs based on 2445f63 :

• Improve ticket registry performance (7.0.x) #6147 for CAS 7.0.x
• Improve ticket registry performance (7.1.x) #6148 for CAS 7.1.x