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

Fix: OpenAPI warnings #270

Merged
merged 9 commits into from
Aug 24, 2023
Merged

Fix: OpenAPI warnings #270

merged 9 commits into from
Aug 24, 2023

Conversation

newstler
Copy link
Contributor

@newstler newstler commented Apr 29, 2023

Closes bullet-train-co/bullet_train#734
Requires bullet-train-co/bullet_train#749

  • Fixes warnings shown by yarn exec redocly lint http://127.0.0.1:3000/api/v1/openapi.yaml
  • Includes Fix: Empty examples in schemas in OpenAPI #236 and Fix: Paths methods generation for OpenAPI #258
  • Test test/controllers/api/open_api_controller_test.rb in starter repo was rewritten to fail on warnings
  • Upgraded jbuilder-schema to >= 2.2.0
  • Added Jbuilder hack module in bullet_train-api/lib/jbuilder/values_transformer.rb to modify attributes produced by external classes, by now it replaces ActionText::RichText object with it's body value. More can be added if needed.

@newstler newstler marked this pull request as draft April 29, 2023 15:07
@newstler newstler force-pushed the fixes/openapi-warnings branch 2 times, most recently from 677da28 to 93b8f3b Compare May 5, 2023 17:18
@newstler newstler marked this pull request as ready for review May 5, 2023 17:23
Copy link
Contributor

@gazayas gazayas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 The Open API controller test wasn't passing for me before this PR, but now it's good.


module ValuesTransformer
def _set_value(key, value)
value = value.body if value.is_a?(ActionText::RichText)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me of the comment in the Transformer about rich text attributes

# TODO The serializers can't handle these `has_rich_text` attributes.

We skip trix editor attributes when super scaffolding jbuilder files, but should we add the body somehow over there in the transformer as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, anyway those attributes are exposed for ScaffoldingCompletelyConcreteTangibleThing when HIDE_THINGS is not enabled. So this Jbuilder hack lets us show ActionText::RichText attributes properly (as string instead of whole object). I think if those were skipped before, now they can be added. Also, if there are any other attributes that should be exposed differently in Jbuilder, they might also be added to this file.

I afraid I didn't fully understand what you meant by should we add the body somehow over there in the transformer as well? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I afraid I didn't fully understand what you meant by should we add the body somehow over there in the transformer as well? :)

I just wasn't sure how we should handle rich text attributes with the API, but after looking into it a bit, I'm thinking I'll make a different PR to return the ActionText::RichText object as a whole instead of just the body (that seems to make the most sense to me). As far as what we return, I think we can leave the decision up to Andrew. I'm fine with how things are now though. Thanks!

Here is an example of bar as a trix editor attribute:
Screenshot from 2023-05-08 20-22-11

Copy link
Contributor Author

@newstler newstler May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that data in API feels completely useless for me, as it is the internal ActionText stuff, and as an API user I can't do anything with that, it's only clutters up the information.

Also, I am pretty sure warnings were there as when we create a record, we send bar as a string. And I'm not sure we now support different types of same attribute for Attributes and Parameters. Should we?

What about the case when some code gets data from one API endpoint and redirects it to another? It will take manual work in the middle to clarify this data..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that data in API feels completely useless for me, as it is the internal ActionText stuff, and as an API user I can't do anything with that, it's only clutters up the information.

Good point, I didn't consider this. I hadn't run the the ActionText::RichText API object against the OpenAPI test so that makes sense about the warnings. Thanks for explaining!

@newstler
Copy link
Contributor Author

newstler commented May 6, 2023

@gazayas Try with Open API controller test from the corresponding branch in starter repo, it was rewritten.

I also updated description.
Not sure if it's right, but this PR includes 2 other PRs that it needs to work correctly :)

@jagthedrummer
Copy link
Contributor

@newstler Can you look at fixing the test failures here? They seem to be related to the OpenAPI stuff that's changed in this PR and/or the one in the starter repo.

@newstler
Copy link
Contributor Author

@jagthedrummer Fixed!

@jagthedrummer jagthedrummer merged commit 69c493a into main Aug 24, 2023
1 check passed
@jagthedrummer jagthedrummer deleted the fixes/openapi-warnings branch August 24, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix OpenAPI warnings for system test
3 participants