Skip to content

Fix Host OpenApi spec - #3635

Merged
balanza merged 1 commit into
mainfrom
fix-host-openapi
Jul 14, 2025
Merged

balanza merged 1 commit into
mainfrom
fix-host-openapi

Conversation

@balanza

@balanza balanza commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

cluster_id is, in fact, nullable for hosts, as they do not always belong to a cluster.

@balanza
balanza requested a review from Copilot July 14, 2025 07:44
@balanza
balanza force-pushed the fix-host-openapi branch from 144809e to e4fa111 Compare July 14, 2025 07:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Host OpenAPI schema to allow cluster_id to be nullable and adds a test to ensure the hosts endpoint handles null cluster_id values correctly.

  • Mark cluster_id as nullable in the OpenAPI schema
  • Add controller test for hosts response when cluster_id is null

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/trento_web/controllers/v1/host_controller_test.exs Added test case to verify handling of null cluster_id
lib/trento_web/openapi/v1/schema/host.ex Marked cluster_id field as nullable: true in schema
Comments suppressed due to low confidence (2)

test/trento_web/controllers/v1/host_controller_test.exs:38

  • Consider adding an explicit assertion to verify that the returned host’s cluster_id is null in the JSON response, for example:
response = get(conn, "/api/v1/hosts") |> json_response(200)
assert Enum.all(response["data"], fn host -> host["cluster_id"] == nil end)
      |> json_response(200)

lib/trento_web/openapi/v1/schema/host.ex:105

  • [nitpick] It may be helpful to update the description to mention that cluster_id can be null (e.g., "Identifier of the cluster this host is part of, or null if it does not belong to a cluster") to make the API contract clearer for consumers.
            description: "Identifier of the cluster this host is part of",

@nelsonkopliku nelsonkopliku left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch!

@gagandeepb gagandeepb left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An additional caveat is that that we should not rely too much on nullability via OpenAPI. The reason is that there is no support for explicitly nullable fields in OpenAPI version 3.1. (It should be fine for now since we are on a version of the spec that is < 3.1 that has support for nullability and the library we use has limited/probably no upgrade path to spec version 3.1.)

@balanza

balanza commented Jul 14, 2025 •

Copy link
Copy Markdown
Contributor Author

An additional caveat is that that we should not rely too much on nullability via OpenAPI. The reason is that there is no support for explicitly nullable fields in OpenAPI version 3.1. (It should be fine for now since we are on a version of the spec that is < 3.1 that has support for nullability and the library we use has limited/probably no upgrade path to spec version 3.1.)

Should we go for [string, null] instead? cannot do it

@gagandeepb

Copy link
Copy Markdown
Contributor

@balanza It is fine as it is for now. There are other places where we use the explicit nullability that will need to get refactored too if/when we upgrade to spec version 3.1.

@balanza
balanza merged commit a77ea5b into main Jul 14, 2025
@balanza
balanza deleted the fix-host-openapi branch July 14, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants