Closed perlpunk closed 4 years ago
Missing fields should be reported in sorted order.
That's not in the spec though :) If we were to have any sort of order, I think probably the best order is the order that the fields are listed in in the required
key.
Because sort is special, I think it is taking the uniq function as the sorting function
Aha (and doh!)
I just noticed that one problem in using a home-grown uniq
sub, instead of List::Util::uniq (why, anyway? List::Util is in core!) is that it loses the original ordering of the list. :(
I wasn't able to figure out when List::Util::uniq became part of core. Can you please tell me which version of Perl that is? I know List::Util have been there for "ages", but the uniq() functions has not been there that long...
Just learned about corelist -a
👍 So it was added in v5.25.1. That's relatively recent, but old enough for me to add List::Util to Makefile.PL
.
Thanks, explicitly defining the sort function via { $a cmp $b }
also fixes the issue.
Not sure why you say JSON::Validator::Util::uniq
is losing the the original order, @karenetheridge . To me it looks like it works correctly, just that the call of sort
was wrong.
Just learned about
corelist -a
See also http://perlpunks.de/corelist :)
Missing fields should be reported in sorted order.
That's not in the spec though :) If we were to have any sort of order, I think probably the best order is the order that the fields are listed in in the
required
key.
Well, it might not be in 'the spec", but the code has already a call to sort
, just that it was broken.
So I don't understand "If we were to have any sort of order". Apparently the intention was there to alphabetically sort the fields.
btw, here's a demonstration that just switching to List::Util::uniq
wouldn't have fixed the issue:
% perl -wE'
use List::Util qw/ uniq /;
my @array = qw/ a z b b y c x /;
my @sorted = sort uniq @array;
say "Sorted: @sorted";'
Argument "z" isn't numeric in sort at -e line 4.
Argument "b" isn't numeric in sort at -e line 4.
Argument "c" isn't numeric in sort at -e line 4.
Argument "x" isn't numeric in sort at -e line 4.
Argument "y" isn't numeric in sort at -e line 4.
Argument "b" isn't numeric in sort at -e line 4.
Sorted: a z b b y c x
The result is neither unique nor sorted, because uniq($a, $b)
is called by sort
, instead of the default { $a cmp $b }
, and instead of returning -1, 0 or 1, it returns items of the list.
Only specifying the sort function (or using parens would have) fixed it:
my @sorted = sort { $a cmp $b } uniq @array;
my @sorted = sort (uniq (@array));
I just wanted to add that to make the actual issue clear, because I saw that the Changelog only mentioned changing the uniq
function.
Thanks for the quick release!
Btw, I'm wondering - there seems to be no test for the order of the fields. Not sure if I have the time to add one in the next weeks, but I think it should be added.
sorry, I forgot that uniq was added to List::Util fairly recently and therefore wouldn't be (automatically) available in earlier perl installs, without upgrading List::Util!
So I don't understand "If we were to have any sort of order". Apparently the intention was there to alphabetically sort the fields.
Previously, the implementation of uniq
was to return the elements in random order, because it used a hash internally to remove duplicate fields, and hash ordering is random in perl (and nondeterministic since 5.17.6). Therefore, testing error messages would be problematic without ordering them in some way.
Now, with List::Util::uniq, we can preserve the original order of the elements, and so (IMHO) we should be doing that, since requires
is a list in json schema. The user can order the requires
fields any way they like, whether alphabetically, or in order of importance, etc. We can and should preserve that order in the responses rather than alphabetically sorting.
After I resolve the current set of outstanding pull requests, I think I might send another to change this, as there are a few different places in the code that are relevant.
Previously, the implementation of
uniq
was to return the elements in random order, because it used a hash internally to remove duplicate fields
Hm: https://github.com/mojolicious/json-validator/blob/353605e4cb86f85eee95e3b215748ade1e936fee/lib/JSON/Validator/Util.pm#L147-L150 To me it looks like it was keeping the order.
we can preserve the original order of the elements
I agree
Steps to reproduce the behavior
I don't have a code example yet, but the order of missing required fields changed. Before they were sorted.
Expected behavior
Missing fields should be reported in sorted order.
Actual behavior
Missing fields are reported in apparently random order.
I think the culprit is this line: https://github.com/mojolicious/json-validator/blob/master/lib/JSON/Validator.pm#L899
Because
sort
is special, I think it is taking theuniq
function as the sorting function. This can be fixed by using parens: