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

Fix: OpenAPI warnings #270

Merged
merged 9 commits into from
Aug 24, 2023
Merged

Fix: OpenAPI warnings #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
29b1b63
Fix: Send FactoryBot examples to JbuilderSchema for proper attributes…
newstler Apr 9, 2023
d42ca60
Fix for unwrapped parameters in examples
newstler Apr 29, 2023
5ff77b5
Fix Standard
newstler Apr 29, 2023
786d295
Fix id type in paths
newstler Apr 30, 2023
8fb890c
Upgrade Ruby to 3.2.2
newstler May 1, 2023
76d2f7f
Hacked Jbuilder to correct action_text values
newstler May 2, 2023
84e0a2e
Rename Jbuilder Hacks module to ValuesTransformer
newstler May 5, 2023
b1126d5
Updated jbuilder-schema
newstler May 5, 2023
c6cbef4
Fix: namespaced model names transform to undercored
newstler Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,4 @@ RUBY VERSION
ruby 3.2.2p53

BUNDLED WITH
2.3.8
2.4.12
5 changes: 3 additions & 2 deletions bullet_train-api/app/helpers/api/open_api_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def automatic_components_for(model, locals: {})
}.merge(locals))

schema_json = jbuilder.json(
model.new,
FactoryBot.example(model.model_name.param_key.to_sym) || model.new,
title: I18n.t("#{model.name.underscore.pluralize}.label"),
# TODO Improve this. We don't have a generic description for models we can use here.
description: I18n.t("#{model.name.underscore.pluralize}.label"),
Expand Down Expand Up @@ -92,7 +92,8 @@ def automatic_components_for(model, locals: {})

parameters_output = JSON.parse(schema_json)
parameters_output["required"].select! { |key| strong_parameter_keys.include?(key.to_sym) }
parameters_output["properties"].select! { |key, value| strong_parameter_keys.include?(key.to_sym) }
parameters_output["properties"].select! { |key| strong_parameter_keys.include?(key.to_sym) }
parameters_output["example"]&.select! { |key, value| strong_parameter_keys.include?(key.to_sym) && value.present? }

(
indent(attributes_output.to_yaml.gsub("---", "#{model.name.gsub("::", "")}Attributes:"), 3) +
Expand Down
4 changes: 2 additions & 2 deletions bullet_train-api/app/views/api/v1/open_api/shared/_paths.yaml.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
in: path
required: true
schema:
type: string
type: integer
- $ref: "#/components/parameters/after"
responses:
"404":
Expand Down Expand Up @@ -44,7 +44,7 @@
in: path
required: true
schema:
type: string
type: integer
requestBody:
description: "Information about a new Tangible Thing"
required: true
Expand Down
2 changes: 1 addition & 1 deletion bullet_train-api/bullet_train-api.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Gem::Specification.new do |spec|
spec.add_dependency "pagy_cursor"
spec.add_dependency "rack-cors"
spec.add_dependency "doorkeeper"
spec.add_dependency "jbuilder-schema", ">= 2.0.0"
spec.add_dependency "jbuilder-schema", ">= 2.2.0"
spec.add_dependency "factory_bot"

spec.add_dependency "bullet_train"
Expand Down
1 change: 1 addition & 0 deletions bullet_train-api/lib/bullet_train/api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
require "scaffolding/block_manipulator"
require "scaffolding/transformer"
require "jbuilder/schema"
require "jbuilder/values_transformer"

module BulletTrain
module Api
Expand Down
5 changes: 4 additions & 1 deletion bullet_train-api/lib/bullet_train/api/example_bot.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def _path_examples(method, model, **options)
else
template, class_name, var_name, values = _set_values("get_example", model)

unless %w[example examples].include?(method.split("_").last)
if method.end_with?("parameters")
if has_strong_parameters?("::Api::#{version.upcase}::#{class_name.pluralize}Controller".constantize)
strong_params_module = "::Api::#{version.upcase}::#{class_name.pluralize}Controller::StrongParameters".constantize
strong_parameter_keys = BulletTrain::Api::StrongParametersReporter.new(class_name.constantize, strong_params_module).report
Expand All @@ -103,6 +103,9 @@ def _path_examples(method, model, **options)
parameters_output = JSON.parse(output)
parameters_output&.select! { |key| strong_parameter_keys.include?(key.to_sym) }

# Wrapping the example as parameters should be wrapped with the model name:
parameters_output = {model.to_s => parameters_output}

return indent(parameters_output.to_yaml.delete_prefix("---\n"), 6).html_safe
end
return nil
Expand Down
13 changes: 13 additions & 0 deletions bullet_train-api/lib/jbuilder/values_transformer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

require "jbuilder"

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

bullet_train-core/bullet_train-super_scaffolding/lib/scaffolding/transformer.rb

Line 1140 in aad0748

# 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 •
edited
Loading

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!


super(key, value)
end
end

::Jbuilder.prepend ValuesTransformer
Loading