Open
Conversation
agrare
reviewed
Sep 15, 2025
Comment on lines
29
to
43
| def get_model_class(klass) | ||
| case klass | ||
| when String | ||
| begin | ||
| klass.constantize | ||
| rescue NameError => e | ||
| puts "Warning: Could not constantize #{klass}: #{e.message}" | ||
| nil | ||
| end | ||
| when Class | ||
| klass | ||
| else | ||
| nil | ||
| end | ||
| end |
Member
There was a problem hiding this comment.
This looks like it could be just klass = klass.safe_constantize if klass.kind_of?(String)
If we don't need to handle Class or String and only String then we could use safe_constantize directly.
Member
There was a problem hiding this comment.
NOTE I threw a breakpoint in else and Class and no instances of that so I think we can do collection.klass&.safe_constantize below
Fryguy
reviewed
Sep 15, 2025
| "properties" => { | ||
| "name" => {"type" => "string", "example" => collection_name}, | ||
| "count" => {"type" => "integer", "description" => "Total number of resources"}, | ||
| "subcount" => {"type" => "integer", "description" => "Number of resources returned"}, |
Member
There was a problem hiding this comment.
Very minor
Suggested change
| "subcount" => {"type" => "integer", "description" => "Number of resources returned"}, | |
| "subcount" => {"type" => "integer", "description" => "Number of resources returned after filters applied"}, |
Fryguy
reviewed
Sep 15, 2025
| "type" => "object", | ||
| "properties" => { | ||
| "name" => {"type" => "string", "example" => collection_name}, | ||
| "count" => {"type" => "integer", "description" => "Total number of resources"}, |
Member
There was a problem hiding this comment.
Suggested change
| "count" => {"type" => "integer", "description" => "Total number of resources"}, | |
| "count" => {"type" => "integer", "description" => "Total number of resources without filters applied"}, |
Fryguy
reviewed
Sep 15, 2025
Comment on lines
194
to
196
| "404" => {"description" => "Resource not found"}, | ||
| "401" => {"description" => "Unauthorized"}, | ||
| "403" => {"description" => "Forbidden"} |
Member
There was a problem hiding this comment.
Very minor, but let's sort these.
Suggested change
| "404" => {"description" => "Resource not found"}, | |
| "401" => {"description" => "Unauthorized"}, | |
| "403" => {"description" => "Forbidden"} | |
| "401" => {"description" => "Unauthorized"}, | |
| "403" => {"description" => "Forbidden"}, | |
| "404" => {"description" => "Resource not found"} |
agrare
reviewed
Sep 15, 2025
Comment on lines
408
to
409
| collection.respond_to?(:collection_actions) && | ||
| collection.collection_actions&.key?(action) |
Member
There was a problem hiding this comment.
Suggested change
| collection.respond_to?(:collection_actions) && | |
| collection.collection_actions&.key?(action) | |
| collection.respond_to?(:collection_actions) && | |
| collection.collection_actions&.key?(action) |
agrare
reviewed
Sep 15, 2025
Comment on lines
413
to
414
| collection.respond_to?(:resource_actions) && | ||
| collection.resource_actions&.key?(action) |
Member
There was a problem hiding this comment.
Suggested change
| collection.respond_to?(:resource_actions) && | |
| collection.resource_actions&.key?(action) | |
| collection.respond_to?(:resource_actions) && | |
| collection.resource_actions&.key?(action) |
Fryguy
reviewed
Sep 15, 2025
agrare
reviewed
Sep 15, 2025
| def build_update_schema(model_class, schema_name) | ||
| properties = build_writable_properties(model_class) | ||
| # Make all properties optional for PATCH | ||
| properties.each { |_, prop| prop.delete("required") if prop.is_a?(Hash) } |
Member
There was a problem hiding this comment.
Suggested change
| properties.each { |_, prop| prop.delete("required") if prop.is_a?(Hash) } | |
| properties.each_value { |prop| prop.delete("required") if prop.is_a?(Hash) } |
Fryguy
reviewed
Sep 15, 2025
Comment on lines
418
to
422
| if collection.respond_to?(:resource_actions) && collection.resource_actions | ||
| collection.resource_actions.keys | ||
| else | ||
| [] | ||
| end |
Member
There was a problem hiding this comment.
Suggested change
| if collection.respond_to?(:resource_actions) && collection.resource_actions | |
| collection.resource_actions.keys | |
| else | |
| [] | |
| end | |
| collection.try(:resource_actions)&.keys || [] |
agrare
reviewed
Sep 15, 2025
Comment on lines
65
to
69
| next unless collection.klass | ||
|
|
||
| # Skip if we can't get a valid model class | ||
| model_class = get_model_class(collection.klass) | ||
| next unless model_class |
Member
There was a problem hiding this comment.
If this is always a string we could do
model_class = collection.klass&.safe_constantize
next if model_class.nil?
Fryguy
reviewed
Sep 15, 2025
Comment on lines
470
to
471
| "subcoll_id" => { | ||
| "name" => "subcoll_id", |
Member
There was a problem hiding this comment.
I think this should be spelled out here.
Suggested change
| "subcoll_id" => { | |
| "name" => "subcoll_id", | |
| "subcollection_id" => { | |
| "name" => "subcollection_id", |
agrare
reviewed
Sep 15, 2025
| # POST collection - if creation is supported | ||
| if supports_collection_action?(collection, :post) | ||
| operations["post"] = { | ||
| "summary" => "Create #{collection_name.to_s.singularize}", |
Member
There was a problem hiding this comment.
I think we need to check the collection_actions here because not every POST on the collection creates the resource.
Example: VMs has a lot of collection_actions but none of them are :name: create
Author
There was a problem hiding this comment.
Hi @agrare, I request your input on this to proceed further.
Member
There was a problem hiding this comment.
I think it could be similar to the resource_actions you have down here https://github.com/ManageIQ/manageiq-api/pull/1297/files#diff-d14d325d93f1febc7f776ea58816bce4c5b74cd4b3c09b883a2dd2c82889857bR289
Author
There was a problem hiding this comment.
like this @agrare?,
# POST collection - only if creation is supported
if collection.collection_actions&.key?(:create)
operations["post"] = {
"summary" => "Create #{collection_name.to_s.singularize}",
"description" => "Create a new #{collection_name.to_s.singularize}",
"requestBody" => {
"required" => true,
"content" => {
"application/json" => {
"schema" => build_create_schema(model_class, schema_name)
}
}
},
"responses" => {
"201" => {
"description" => "Resource created successfully",
"content" => {
"application/json" => {
"schema" => {"$ref" => "##{SCHEMAS_PATH}/#{schema_name}"}
}
}
},
"400" => {"description" => "Bad Request - Invalid input"},
"401" => {"description" => "Unauthorized"},
"403" => {"description" => "Forbidden"},
"422" => {"description" => "Unprocessable Entity - Validation errors"}
},
"tags" => [collection_name.to_s.titleize]
}
end
# Document other collection_actions (excluding :create) as POSTs
collection.collection_actions&.each do |action, config|
next if action == :create
operations["post_#{action}"] = {
"summary" => "Invoke #{action.to_s.titleize} on #{collection_name}",
"description" => "Perform the #{action} action on the #{collection_name} collection.",
"requestBody" => {
"required" => true,
"content" => {
"application/json" => {
"schema" => {
"type" => "object",
"required" => ["action"],
"properties" => {
"action" => {
"type" => "string",
"enum" => [action.to_s],
"description" => "Name of the collection action"
},
# Optionally add custom params from config if needed
}
}
}
}
},
"responses" => {
"200" => {"description" => "Action performed successfully"},
"400" => {"description" => "Bad Request - Invalid action or parameters"},
"401" => {"description" => "Unauthorized"},
"403" => {"description" => "Forbidden"},
"422" => {"description" => "Action cannot be performed"}
},
"tags" => [collection_name.to_s.titleize]
}
end
Member
There was a problem hiding this comment.
Do we need to special case create there or can we treat it like all of the other collection actions?
Contributor
There was a problem hiding this comment.
Yes @agrare, We can handle create the same way as the other collection actions
Member
|
This is an awesome PR @dhamo-ror. Thank you! Can you add the generated openapi.yml to this PR as well, which is the result of running these updates? That way we can also see the final result. Also, not necessarily for this PR, but in the future we also need OPTIONS support for the endpoints, and we need to also add the |
Author
|
Thanks @Fryguy for your review. I’ll address the comments, update the PR and share the YAML file as well. I also plan to include all actions and endpoints in this, so please share your inputs if I’ve missed anything. |
Author
|
Hi @Fryguy, I’m attaching the JSON file generated for the API. Please take a look and share your feedback. |
Member
|
@dhamo-ror It should just be a part of this PR is what I mean. We have the results of running the generator in https://github.com/ManageIQ/manageiq-api/blob/master/config/openapi.json |
Author
|
Understood @Fryguy, Once we finalize the |
Member
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
Member
|
Checked commits dhamo-ror/manageiq-api@3706e6c~...dc405f6 with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint lib/manageiq/api/open_api/generator.rb
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @agrare, could you kindly review this PR and let me know your suggestions.