Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

swagger-parser-v3 can throw NPE due to non-initialized instance fields. #682

Open
dhoffer opened this issue Apr 11, 2018 · 40 comments
Open

swagger-parser-v3 can throw NPE due to non-initialized instance fields. #682

dhoffer opened this issue Apr 11, 2018 · 40 comments

Comments

@dhoffer
Copy link

dhoffer commented Apr 11, 2018

swagger-parser-v3 can throw NPE parsing/loading Open API spec. The NPE is caused by io.swagger.v3.parser.util#ResolverFully because the schemas and examples fields are not initialized in all cases.

Version 2.0.0-rc3.

Our local fix was to just always initialize all the private fields maps with empty HashMap.
@webron
Copy link
Contributor

webron commented Apr 11, 2018

It would help to get an API definition to reproduce it.

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

I have attached a sample but you can see in that class that only 1 of the instance fields is initialized in some cases.

Note this parser seems incomplete in may ways as we have found several bugs. Is there a better Open API v3 parser that we should be using? I am not using this component directly but through another component that has this as a dependency. Should they be using a different more complete parser? I also noted this is an RC version when will this be released as final non-RC?

openapi_sample.zip

@gracekarina
Copy link
Contributor

Hi it is possible some of this issues have been solved in the work done in parser after rc3, can you please try with the snapshot?. Thanks!

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

What is the version of the new snapshot, I'm not finding it.

@gracekarina
Copy link
Contributor

2.0.0-SNAPSHOT.

@gracekarina
Copy link
Contributor

can you please send this spec:
'./resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json'

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

this is part of the file: and it calls the route : './resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json',

paths:
  /JTasker/startRun:
    post:
      tags:
        - JTasker
      summary: Start JTasker Run
      operationId: JTasker_startRun
      requestBody:
        content:
          application/json:
            schema:
              $ref: './resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json'
        required: true
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                type: array
                items:
                  type: string
        '500':
          description: Server Error

@gracekarina
Copy link
Contributor

by the way the ref it's built I assume that ./resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json' it has just schemas inside.

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

Yes that is correct. It is the schema for that requestBody.

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

gracekarina commented Apr 11, 2018 •
edited
Loading

yes, so I need that file, the one with the schema. or is it the same? I see the file is a json and you sent us a yaml

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

yess, sorry! I'll check again

@gracekarina
Copy link
Contributor

gracekarina commented Apr 11, 2018 •
edited
Loading

Hi!. In 2.0.0-SNAPSHOT the test passes this is the output after parsing with resolveFully:

  tags:
  - JTasker
  summary: Start JTasker Run
  operationId: JTasker_startRun
  requestBody:
    content:
      application/json:
        schema:
          type: object
          properties:
            createPlanEvents:
              type: object
              properties:
                name:
                  type: string
                description:
                  type: string
                events:
                  type: array
                  items:
                    type: string
            createPlanSensors:
              type: object
              properties:
                sensors:
                  type: array
                  items:
                    type: string
    required: true
  responses:
    200:
      description: Success
      content:
        application/json:
          schema:
            type: array
            items:
              type: string
    500:
      description: Server Error

@gracekarina
Copy link
Contributor

this is the test

 @Test
    public void issues() throws Exception {
        OpenAPIV3Parser parser = new OpenAPIV3Parser();
        ParseOptions options = new ParseOptions();
        options.setResolveFully(true);
        final SwaggerParseResult result = parser.readLocation("src/test/resources/odin.yaml", null, options);
        Assert.assertNotNull(result.getOpenAPI());

        Yaml.prettyPrint(result.getOpenAPI().getPaths().get("/JTasker/startRun"));
        Assert.assertTrue(result.getMessages().isEmpty());

    }

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

it does:

paths:
  /JTasker/startRun:
    post:
      tags:
        - JTasker
      summary: Start JTasker Run
      operationId: JTasker_startRun
      requestBody:
        content:
          application/json:
            schema:
              $ref: './resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json'
        required: true
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                type: array
                items:
                  type: string
        '500':
          description: Server Error

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

Okay I see that one does. Let me see how you are configuring the ParseOptions. I will see if this is different. Note I am not using this directly but indirectly with other 3rd party component. I will have to check how they are configuring your parser.

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

The prior configuration of ParseOptions was like this:
ParseOptions options = new ParseOptions();
options.setResolve(true);
options.setResolveCombinators(false);
options.setResolveFully(true);

I have changed their configuration to be like yours (only setResolveFully is set true) but it still fails with this error: java.lang.RuntimeException: Could not find ./resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json on the classpath

I think there is a problem with your test setup. Make sure the relative resources are in subfolder to spec file. E.g. make sure your tests have this directory structure:
webroot/odin.yaml
webroot/resources/urn_jsonschema_com_issinc_odin_display_CreatePlan.json

Also when running tests in Maven you would normally load resources like this:
getClass().getResource(/webroot/odin.yaml) as by definition everything in src/test/resources is in the test classpath. If you add 'src/test/resources/' prefix then that is not a valid classpath value.

Note our use case is to load the spec file like this:
parser.readLocation("/webroot/odin-ui-openapi-v3.yml", null, options);

And we are loading '/webroot/odin-ui-openapi-v3.yml' from the classpath.

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018

Yeah I have tracked this down some more. If I convert my valid classpath spec resource to a file URI then it works. So the problem appears to be that the classpath loading in the parser is completely broken. As I mentioned above your test case would not be a valid classpath test case.

@gracekarina
Copy link
Contributor

@dhoffer, Please try this sample, it is done with the files you sent. it is successful
parser-cp-loader.zip

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

gracekarina commented Apr 11, 2018 •
edited
Loading

we'll inflector's 2.0 is using:
https://github.com/swagger-api/swagger-inflector/blob/2.0/pom.xml#L300-L304

            <groupId>io.swagger.parser.v3</groupId>
            <artifactId>swagger-parser</artifactId>
            <version>${swagger-parser-version}</version>
        </dependency>

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

where are you trying? which OS?

@gracekarina
Copy link
Contributor

swagger-parser and swagger-parser-v3 are modules of the same project and share lifecycle, 2.0.0-SNAPSHOT is the latest version; swagger-parser also supports v2 spec, for the rest is equivalent to swagger-parser-v3 which uses internally for 3.0 spec

@dhoffer
Copy link
Author

dhoffer commented Apr 11, 2018 via email

@gracekarina
Copy link
Contributor

ok, let me check another OS.

@gracekarina
Copy link
Contributor

gracekarina commented Apr 13, 2018 •
edited
Loading

parser-cp-loader.zip
Hi @dhoffer sending sample, tested succesfully on windows.
Please let us know if this is still an issue.
Also added a test with the spec provided to parser.
Regards.

gracekarina added a commit that referenced this issue Apr 13, 2018
@gracekarina
adding test for issue #682
e03531e
@gracekarina gracekarina mentioned this issue Apr 13, 2018
Merged
gracekarina added a commit that referenced this issue Apr 13, 2018
@gracekarina
Merge pull request #686 from swagger-api/classpath-test
8b27bf3 
Adding test for issue #682 - ClassPath
@dhoffer
Copy link
Author

dhoffer commented Apr 13, 2018

You are not testing the classpath usecase with your test sample you just sent. Look at your code. In the Controller you have this:
String LOCATION = getClass().getResource("/spec/ofSpec/odin-ui-openapi-v3.yaml").toString();

That converts a classpath value to a file URI. Look at the value of LOCATION in your debugger. Also you can see when you step through the code you are loading that as a file not from class path.

Change that string to be the following to keep it a valid class path value.
String LOCATION = "/spec/ofSpec/odin-ui-openapi-v3.yaml";

And you will see the problem. Your class path code in this artifact is completely broken. You have to properly configure your tests so you are testing the case that does not work.

@gracekarina
Copy link
Contributor

gracekarina commented Apr 13, 2018 •
edited
Loading

this comment you made before:

Also when running tests in Maven you would normally load resources like this:
getClass().getResource(/webroot/odin.yaml)

@gracekarina
Copy link
Contributor

gracekarina commented Apr 13, 2018 •
edited
Loading

The idea is that you change the artifactId in your project and let us know how that goes. I sent the sample in order to show you the proper way to setup parser.
Also please check the test in parser I added.

@dhoffer
Copy link
Author

dhoffer commented Apr 13, 2018

Yes as I mentioned above your test is not valid for class path resource. getClass().getResource() & getClass().getResourceAsStream() are the way you load class path resources in Java. Since your API accepts any location value (http/file/classpath) those go in the parser not outside in your tests. Obviously if you convert a classpath value to file outside the parser you are not testing the classpath use case anymore.

@webron
Copy link
Contributor

webron commented Apr 13, 2018

@dhoffer you've mentioned that you're not using the library directly but through another component that depends on it. Which one is it? Can you provide a link to it?

gracekarina added a commit that referenced this issue Apr 22, 2018
@gracekarina
adding changes refs #682
e6573c5
@gracekarina gracekarina mentioned this issue Apr 22, 2018
Closed
@dhoffer
Copy link
Author

dhoffer commented Apr 26, 2018

Sorry been swamped, just getting back to this. I have already worked with the other library provider and tracked the issue down to this component so that is not relevant for the issues I have created here.

It turns out my 'workaround' to always use file URI path doesn't work if the file/resource is located in a jar so I had to finally fix your classpath loading issues (which was one if the issues I created). I forked (copied) your latest snapshot and found that the unit test that @gracekarina added is completely wrong. That test must fail when you fix the problem so that is obviously an anti-unit-test.

I do have a workaround that fixes the classpath issue but its not ready for general production so that is our local fix. The problem is there is no much bad code (as well as bad tests) that a significant part of the library would need to be fixed to properly resolve this. I just added some workarounds to properly handle classpath loading with relative resources.

@gracekarina
Copy link
Contributor

@dhoffer Hi, I'm currently working to fix the issue of classpath, including when the file it's on a jar file.
Thanks!.

@webron
Copy link
Contributor

webron commented Apr 26, 2018

@dhoffer while we appreciate the feedback, there are proper ways to deliver it. Throwing around insults doesn't help the library improve, and doesn't help in providing you with a higher quality of a product.

If you'd like to help the project improve, we'd appreciate filing specific tickets on issues you've seen, and we'll gladly look into them and fix whatever is possible. Like our other open source projects, and open source projects in general, we depend a lot on the community to provide us with the problems they encounter so we can address them. We appreciate fixes as well, but also understand it's time consuming and can't always be provided.

However, your phrasing throughout this thread is very unfavorable, and while I refrained from addressing it until now, it's time to voice it.

I bring this up not to rattle any cages, but rather in hopes that we can minimize the negativity and work together to get a better product that would benefit all.

@gracekarina gracekarina mentioned this issue May 3, 2018
Closed
@gracekarina
Copy link
Contributor

fixed by #1134 issue related to issue #1133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@webron @dhoffer @gracekarina