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.

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

Super scaffolding any model causes the OpenAPI test to start failing #1520

Open
jagthedrummer opened this issue Jun 7, 2024 · 6 comments
Open

Comments

@jagthedrummer
Copy link
Contributor

  • Clone the starter repo and run bin/configure & bin/setup
  • rails test - everything passes
  • rails g super_scaffold Widget Team name:text_field
  • rails db:migrate
  • rails test - The OpenAPI test fails

Here's the complete output of running just that test after super scaffolding a simple model. Seems like we should be able to generate models and whatever they need to pass this basic test.

$ rails test test/controllers/api/open_api_controller_test.rb
DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <top (required)> at /Users/jgreen/projects/bullet-train-co/open-api-test-project/config/environment.rb:5)
DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <top (required)> at /Users/jgreen/projects/bullet-train-co/open-api-test-project/config/environment.rb:5)
🌱 Generating global seeds.
🌱 Generating test environment seeds.
Not requiring Knapsack Pro.
If you'd like to use Knapsack Pro make sure that you've set the environment variable KNAPSACK_PRO_CI_NODE_INDEX
Started with run options --seed 55442

Api::OpenApiControllerTest
validating ./tmp/openapi.yaml...
[1] tmp/openapi.yaml:11:10 at #/servers/0/url

Server `url` should not point to example.com or localhost.

 9 |   version: "V1"
10 | servers:
11 |   - url: http://localhost:3000/api/v1
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 | components:
13 |   securitySchemes:

Warning was generated by the no-server-example.com rule.


[2] tmp/openapi.yaml:2031:19 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json/example/0/id

Example value must conform to the schema: `id` property type must be string.

2029 |     $ref: "#/components/schemas/WidgetAttributes"
2030 | example:
2031 |   - id:
     |     ^^^
2032 |     team_id:
2033 |     name: MyString

referenced from tmp/openapi.yaml:2026:15 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[3] tmp/openapi.yaml:2032:19 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json/example/0/team_id

Example value must conform to the schema: `team_id` property type must be string.

2030 | example:
2031 |   - id:
2032 |     team_id:
     |     ^^^^^^^^
2033 |     name: MyString
2034 |     created_at:

referenced from tmp/openapi.yaml:2026:15 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[4] tmp/openapi.yaml:2036:19 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json/example/1/id

Example value must conform to the schema: `id` property type must be string.

2034 |   created_at:
2035 |   updated_at:
2036 | - id:
     |   ^^^
2037 |   team_id:
2038 |   name: MyString

referenced from tmp/openapi.yaml:2026:15 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[5] tmp/openapi.yaml:2037:19 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json/example/1/team_id

Example value must conform to the schema: `team_id` property type must be string.

2035 |   updated_at:
2036 | - id:
2037 |   team_id:
     |   ^^^^^^^^
2038 |   name: MyString
2039 |   created_at:

referenced from tmp/openapi.yaml:2026:15 at #/paths/~1teams~1{team_id}~1widgets/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[6] tmp/openapi.yaml:2077:17 at #/paths/~1teams~1{team_id}~1widgets/post/responses/201/content/application~1json/example/id

Example value must conform to the schema: `id` property type must be string.

2075 |   $ref: "#/components/schemas/WidgetAttributes"
2076 | example:
2077 |   id:
     |   ^^^
2078 |   team_id:
2079 |   name: MyString

referenced from tmp/openapi.yaml:2074:15 at #/paths/~1teams~1{team_id}~1widgets/post/responses/201/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[7] tmp/openapi.yaml:2078:17 at #/paths/~1teams~1{team_id}~1widgets/post/responses/201/content/application~1json/example/team_id

Example value must conform to the schema: `team_id` property type must be string.

2076 | example:
2077 |   id:
2078 |   team_id:
     |   ^^^^^^^^
2079 |   name: MyString
2080 |   created_at:

referenced from tmp/openapi.yaml:2074:15 at #/paths/~1teams~1{team_id}~1widgets/post/responses/201/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[8] tmp/openapi.yaml:2102:17 at #/paths/~1widgets~1{id}/get/responses/200/content/application~1json/example/id

Example value must conform to the schema: `id` property type must be string.

2100 |   $ref: "#/components/schemas/WidgetAttributes"
2101 | example:
2102 |   id:
     |   ^^^
2103 |   team_id:
2104 |   name: MyString

referenced from tmp/openapi.yaml:2099:15 at #/paths/~1widgets~1{id}/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[9] tmp/openapi.yaml:2103:17 at #/paths/~1widgets~1{id}/get/responses/200/content/application~1json/example/team_id

Example value must conform to the schema: `team_id` property type must be string.

2101 | example:
2102 |   id:
2103 |   team_id:
     |   ^^^^^^^^
2104 |   name: MyString
2105 |   created_at:

referenced from tmp/openapi.yaml:2099:15 at #/paths/~1widgets~1{id}/get/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[10] tmp/openapi.yaml:2139:17 at #/paths/~1widgets~1{id}/put/responses/200/content/application~1json/example/id

Example value must conform to the schema: `id` property type must be string.

2137 |   $ref: "#/components/schemas/WidgetAttributes"
2138 | example:
2139 |   id:
     |   ^^^
2140 |   team_id:
2141 |   name: MyString

referenced from tmp/openapi.yaml:2136:15 at #/paths/~1widgets~1{id}/put/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


[11] tmp/openapi.yaml:2140:17 at #/paths/~1widgets~1{id}/put/responses/200/content/application~1json/example/team_id

Example value must conform to the schema: `team_id` property type must be string.

2138 | example:
2139 |   id:
2140 |   team_id:
     |   ^^^^^^^^
2141 |   name: MyString
2142 |   created_at:

referenced from tmp/openapi.yaml:2136:15 at #/paths/~1widgets~1{id}/put/responses/200/content/application~1json

Warning was generated by the no-invalid-media-type-examples rule.


./tmp/openapi.yaml: validated in 71ms

Woohoo! Your API description is valid. 🎉
You have 11 warnings.

  test_OpenAPI_document_is_valid                                  FAIL (2.51s)

Failures and errors:
Api::OpenApiControllerTest
  test_OpenAPI_document_is_valid                                  FAIL (2.51s)
        Expected #<MatchData "You have 11 warnings" 1:"11"> to not be truthy.
        test/controllers/api/open_api_controller_test.rb:20:in `block in <class:OpenApiControllerTest>'

Finished in 2.50896s
1 tests, 1 assertions, 1 failures, 0 errors, 0 skips

It's also worth mentioning that the output from this failure isn't particularly helpful in pointing out what exactly failed. It points to ./tmp/openapi.yaml which is an auto generated file. It would be nice if we could point you towards whatever file you'd need to touch to make changes.

@jagthedrummer
Copy link
Contributor Author

@newstler, wondering if you have any suggestions on how we can handle this better.

@jagthedrummer
Copy link
Contributor Author

After poking around at this for a while I found that I could get the test to pass by updating test/factories/weidgets.rb.

It started like this:

FactoryBot.define do
  factory :widget do
    association :team
    name { "MyString" }
  end
end

And once I did this, the test started passing:

FactoryBot.define do
  factory :widget do
    association :team
    name { "MyString" }
    factory :widget_example do
      id { 42 }
      team { FactoryBot.example(:team) }
    end
  end
end

I think we either need to update the scaffolding process to automatically add the _example factory OR we need to figure out some other way to prevent scaffolding for instantly causing test failures that don't provide any useful info about what's wrong.

@jagthedrummer
Copy link
Contributor Author

An example factory like this also works:

    factory :widget_example do
      id { 42 }
      team_id { 42 }
    end

(Hard coding a team_id instead of using FactoryBot.example to create an example team.

@jagthedrummer
Copy link
Contributor Author

After looking at this some more I think maybe our test is just overly fragile. The "failures" are actually just letting you know about warnings issued by redocly. I'm not sure we should fail the tests just due to warnings.

These are the lines causing the problem:

failures = output.match(/You have (\d+) warnings/) || output.match("Failed to parse api definition")
puts output if failures
refute failures

I'm currently thinking that we shouldn't lump together warnings and failures from redocly and treat them all as failures. Instead I think that we should separate them and only treat redocly failures as failures. And then just output a message if there are warnings.

Maybe something like:

    failures = output.match("Failed to parse api definition")
    puts output if failures
    refute failures

    warnings = output.match(/You have (\d+) warnings/)
    if warnings
      puts "-----------------".yellow
      puts warnings.yellow
      puts "-----------------".yellow
    end

@newstler do you see any problems with doing something like this?

@newstler
Copy link
Contributor

Hey, I’m sorry I haven’t seen that due to absence of notifications on GitHub some why.

You’re right saying that the solution would be to update the scaffolding process to automatically add the _example factory.

I wouldn't change the test itself though, as redocly's warnings are basically OpenAPI errors, meaning the documentation is noncompliant.

@hopsoft
Copy link

hopsoft commented Sep 12, 2024

I'm hitting this issue as well on a new project. I think I have a handle on what it may be and how to fix. Hope to get a PR stood up later today.

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

No branches or pull requests

3 participants