MBS-9129: Adjust JSON serialization for RG ratings on release request. #470
Conversation
|
The better method would be to use I guess the only place to put the variable would be JSON/2/Role/Rating.pm. So you'd need something like this in JSON/2/Release.pm: {
local $MusicBrainz::Server::WebService::Serializer::JSON::2::Role::Rating::show_ratings = 0;
# $show_ratings is only 0 within this block.
}; |
|
Good idea - will force-push a fixup commit using that approach. |
|
That should do it. Tests don't seem to affected. |
| if $inc && $inc->release_groups; | ||
| if ($inc && $inc->release_groups) | ||
| { | ||
| # MBS-9129: If ratings are requested (even though a release doesn't have any), those of the release group should be |
There was a problem hiding this comment.
I prefer always limiting comments to <80 chars per line, since it makes them much easier to read. (Usually code too, but I don't strictly follow that.)
There was a problem hiding this comment.
I'm used to 132 as 80 can get very restrictive (not so much for perl, but for C++ it is), so my emacs is set up for that.
I can reformat those lines, no problem.
|
BTW, is adding a test something you'd be willing to look into? |
|
Sure, I can look at making a test, depending a little on what it all entails. |
|
LGTM. |
Without inc=release-groups, it should be a no-op. Otherwise, the linked RG's ratings are returned. Also tests for an "unauthorised" response when user ratings are requested without proper credentials.
|
Comment fixup squashed. Probably ready for merge now. |
The first commit is straightforward - a release has no ratings so should not try to serialize them.
The second part aims to replicate the XML serializer's behaviour - a request for ratings on a release is "forwarded" to the release's RG (if it's also requested).
However, the JSON serializer is made up of a bunch of disjoint sources, and the bit that handles ratings has no way of knowing it's handling a release group that is being serialized as part of a release request.
If I change MusicBrainz::Server::WebService::Serializer::JSON::2::Role::Rating to ignore the toplevel flag, all release groups (including those in (requested) relationships) in all requests will serialize their ratings; instead I (ab)use $inc to pass a special flag for it to pick up.
I'm not very happy with that, but the only other way I see is to change the basic serialize() signature and either add an extra parameter, or change the semantics of the current $toplevel flag (e.g. by calling it $flags, and changing tests of $toplevel to checks for $flags->{TOPLEVEL}). Other approaches/suggestions welcome.