Skip to content

Fix an issue with conflicting JSON modules in use in FormatRenderedProblem.pm.#2692

Merged
Alex-Jordan merged 3 commits into
openwebwork:developfrom
drgrice1:format-rendered-problem-json-conflict
Apr 5, 2025
Merged

Fix an issue with conflicting JSON modules in use in FormatRenderedProblem.pm.#2692
Alex-Jordan merged 3 commits into
openwebwork:developfrom
drgrice1:format-rendered-problem-json-conflict

Conversation

@drgrice1

@drgrice1 drgrice1 commented Apr 2, 2025

Copy link
Copy Markdown
Member

Both the JSON module and Mojo::JSON::encode_json method are loaded in lib/FormatRenderedProblem.pm. This causes first request issued to the render_rpc endpoint after each server process starts to issue the warning Prototype mismatch: sub FormatRenderedProblem::encode_json ($) vs none at /usr/lib/x86_64-linux-gnu/perl-base/Exporter.pm line 63. Once the modules are loaded in the process they aren't loaded again, so the warning only occurs once per process.

There is no need for both modules that both do essentially the same thing. The Mojo::JSON::encode_json method also UTF-8 encodes the data as was done before.

Note that the only place the JSON module was used was for rendering the raw format.

This is just something I noticed while working on #2691.

…oblem.pm.

Both the `JSON` module and `Mojo::JSON::encode_json` method are
loaded in `lib/FormatRenderedProblem.pm`.  This causes first request
issued to the `render_rpc` endpoint after the server starts to issue the
warning `Prototype mismatch: sub FormatRenderedProblem::encode_json ($)
vs none at /usr/lib/x86_64-linux-gnu/perl-base/Exporter.pm line 63.`

There is no need for both modules that both do essentially the same
thing.  The `Mojo::JSON::encode_json` method also UTF-8 encodes the
data as was done before.

Note that the only place the `JSON` module was used was for rendering
the `raw` format.

@somiaj somiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test, but looks straight forward and obvious change to remove an unneeded module.

I see we use JSON if a few other places, is it worth switching those to Mojo::JSON as well and then remove dependency on JSON?

@drgrice1

drgrice1 commented Apr 2, 2025

Copy link
Copy Markdown
Member Author

Yeah, it would be good to do that, but note that PG also uses the JSON module. So that wouldn't quite remove the dependency.

PG still uses `JSON` for now, so this doesn't quite remove the
dependency on the module.  But PG can be switched also.

Note that the module uses `Cpanel::JSON::XS` (if that module is
installed) which is the recommended perl JSON encoding and decoding
module. `Mojo::JSON` falls back to their own pure perl implementation if
`Cpanel::JSON::XS` is not installed.

Note that the `Mojo::JSON::encode_json` method UTF-8 encodes the output
and is canonical (sorts object keys). The `Mojo::JSON::to_json` method
does not UTF-8 encode the output, but is still canonical.  The
corresponding `Mojo::JSON::decode_json` decodes UTF-8 encoded JSON and
`Mojo::JSON::from_json` method decodes non UTF-8 encoded json.
@drgrice1

drgrice1 commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

I decided to take this where @somiaj suggested. I replace all JSON module usage with Mojo::JSON usage.

@drgrice1

drgrice1 commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

And now this goes all the way. In openwebwork/pg#1226 PG is also switched to using Mojo::JSON.

@Alex-Jordan
Alex-Jordan merged commit 6395021 into openwebwork:develop Apr 5, 2025
@Alex-Jordan

Copy link
Copy Markdown
Contributor

I tested this (too lightly perhaps) on my dev server. Then merged here. Then deployed to production server, and some things broke with graphTool exercises. I do believe I had run npm ci and restarted webwork2.

No time to investigate now, but I will later. I wanted to note that maybe there is an issue here.

@drgrice1

drgrice1 commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

I am sorry, I forgot to mention in this pull request that this is paired with openwebwork/pg#1226. That would be why the graphTool exercises are now broken (among other things).

@drgrice1

drgrice1 commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

I guess I should have waited with that part and made a separate pull request.

@drgrice1

drgrice1 commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

For now you can change line 1366 of conf/defaults.config back to [qw(JSON)] and that will get PG back. That is the only thing in this pull request (other than in bin/check_modules.pl) that PG depends on.

@Alex-Jordan

Copy link
Copy Markdown
Contributor

It's no problem, in production I just used git to undo the merge. We can take time to get pg#1226 in too.

@drgrice1
drgrice1 deleted the format-rendered-problem-json-conflict branch April 8, 2025 23:03
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

Successfully merging this pull request may close these issues.

3 participants