Check that something is a hash ref before you use it as a hashref

[briandfoy]

I have a file that passes the other linters, including the one from Swagger, but openapi-linter blows up:

$ openapi-linter --spec some_file.yml

I get:

Not a HASH reference at /usr/local/perls/perl-5.42.0/lib/site_perl/5.42.0/OpenAPI/Linter.pm line 185.

This comes from code like:

        for my $method (sort keys %{$spec->{paths}{$path}}) {
            my $op = $spec->{paths}{$path}{$method};
            push @issues, {
                level   => 'WARN',
                message => "Missing description for $method $path"
            } unless $op->{description};
        }

You assume that the value in $op is a hash reference. If it isn't, there should be a different validation error. Once you have verified that $op is a hashref, you can move on to the other checks. This is especially important when you expect malformed formats.

There are several other areas in the code where this is also a problem.

[domm]

I got the same error, this fixes it (at least this one case):

    if ($spec->{paths}) {
        for my $path (sort keys %{$spec->{paths}}) {
            for my $method (sort keys %{$spec->{paths}{$path}}) {
                my $ops = $spec->{paths}{$path}{$method};
                if (ref($ops) eq 'HASH') {
                    $ops = [ $ops ];
                }
                for my $op (@$ops) {
                    push @issues, {
                        level   => 'WARN',
                        message => "Missing description for $method $path"
                    } unless $op->{description};
                }
            }
        }
    }

@manwar I could prepare a proper PullRequest if this makes sense to you

[manwar]

@domm Your suggestion looks ok to me, please submit the proposed change as a pull request.

[briandfoy]

Note that the true fix is going to recognize that the value should have been a particular type and warn that it wasn't. If something is supposed to be an array in the OpenAPI spec but it isn't in the file, that's an error the linter needs to communicate.

I think this means that most of the structure of the module needs to change to accomodate that sort of reporting.

[domm]

I haven't looked at what the Spec returns here and assumed it could be a HASH (if it's a single value) or an ARRAY of HASHes (for multiple values).

But I guess it would make a lot of sense to figure out what kind of return values are to be expected here and write code that handles this correctly.

I was just giving the module a try, it failed, and I patched it without much consideration of the context.

Not sure if I have the time to properly investigate the issue, as brian suggests. But if I do submit a PR, I will do some research first :-)

[manwar]

@domm just released patch (v0.16) to address the issue. please give it a try:

[briandfoy]

This patch isn't the right way to do things. I think the entire approach is flawed, and you can't special case this stuff.

[manwar]

@briandfoy please suggest the right way and help me improve the code.

[briandfoy]

I think the best thing you can do is look at the code of existing linters to get an idea of the scope of the problem. I have the feeling that you have underestimated how big the problem is.

I think the first next thing you need to do is figure out either how broken JSON::Validator is, or how you are using it wrong, as ether told you. You've already noted there's a mismatch in what JSON::Validator does and what you see with your module. Consider that JSON::Validator is not what you should be using. From it's docs:

JSON::Validator::Schema or any of the sub classes can be used instead of JSON::Validator. The only reason to use JSON::Validator directly is if you don't know the schema version up front.

Also consider that a checking each node is not enough. You're going to have to do long range checking of refs and other things. Play with a lot of messed up swagger files with other linters and notice how they handle them. Consider all of that when you redesign your code. Expand your corpus of test files for all the situations that you can create to cause problems, as well as a list of known good files that pass the other linters to see if you have problems on those. Don't just test a few edge cases in isolation.

But also consider that if this is too much work for you and if it is this broken, and so much less useful than existing tools, that it's not yet time to promote it or tell people to use it. I don't need this module because much better tools already exist, although a Perl module which is just was good would be very interesting. This is not that module yet.

I've already evaluated three different versions, none of which have fixed the basic problem in this issue. You shouldn't have to ask me if it's fixed, and I shouldn't have to be the one to figure out this is still broken. Your tests should catch all that. Again, run this against every swagger file online that you can find. It shouldn't fail on any of the ones that pass Swagger's tools.