Openapi schemas no additional properties #2355

CDimonaco · 2024-02-21T12:07:10Z

Description

This PR aims to fix the properties of OpenApi Schemas.
This change involves the schemas, controllers and views.
Now our openapi schema is consistent with web responses.

Controller response views and broadcast views are differentiated, in order to achieve consistency on both the transports.

I introduced a new convention the broadcast_ prefix in views means that the views are used ONLY in the broadcast of messages.

Remember to add the right labels to this PR.

Automated tests

compatibility

dottorblaster

I really like the approach of creating dedicated view clauses for broadcasts 👍

arbulu89

Hey @CDimonaco
Great effort.
Unfortunately, there are many things wrong.
I think you have trusted too much in the current state of the schemas.
Many changes you did in the views are wrong, because you based them in the schema, but the schema itself is what it is wrong.
I think I have caught all the things.
Let me know if you need more details.

As a nitpick:
The database and sapsystem view changes. The content we send in the controller and in the broadcast should be the same. It you want to keep the duplicated code, to be explicit, so be it, but with the same content

arbulu89 · 2024-02-22T07:44:44Z

lib/trento_web/views/v1/cluster_view.ex

@@ -8,6 +8,7 @@ defmodule TrentoWeb.V1.ClusterView do
  def render("cluster.json", %{cluster: cluster}) do
    cluster
    |> Map.from_struct()
+    |> Map.delete(:deregistered_at)


Good call.
I don't know even know why we send this value in the host view, we don't use it in the frontend...

arbulu89 · 2024-02-22T07:52:47Z

lib/trento_web/views/v1/health_overview_view.ex

@@ -28,8 +28,6 @@ defmodule TrentoWeb.V1.HealthOverviewView do
      sid: sid,
      sapsystem_health: sapsystem_health,
      database_health: database_health,
-      # deprecated field


we shouldn't be removing this in reality.
the field is still in the schema, even with the deprecated value.
Unless we created a v2 version (which we don't want), we must keep them.
Most probably nobody is using it, but removing it would make the schema non backward compatible.

arbulu89 · 2024-02-22T07:59:53Z

lib/trento_web/views/v1/host_view.ex

+    |> Map.from_struct()
+    |> Map.put(:sles_subscriptions, sles_subscriptions)
+    |> Map.delete(:fully_qualified_domain_name)
+    |> Map.delete(:selected_checks)


Can we remove this selected_checks?
As far as I know it should be used in the frontend.
For me, the error is actually in the schema, that it is not include.
Can you confirm @dottorblaster @nelsonkopliku ?

You're right, we take this and put it in the hosts slice as it is.

Correct, both when first loading the /hosts api and when some event that gets projected changes the selected checks (which we broadcast)

arbulu89 · 2024-02-22T08:02:08Z

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

@@ -51,6 +58,7 @@ defmodule TrentoWeb.OpenApi.V1.Schema.Host do
          type: :string,
          description: "Version of the agent installed on the host"
        },
+        health: ResourceHealth,


Nitpick.
Not about this field.
I have checked a bit the code, and the value we could deprecate is the deregistered_at.
It is not used in the frontend anywhere.
So we could remove it in a potential, v2 version.
Anyway, it is not strictly about this PR. Up to you. If you don't do it, it is not a big deal.

arbulu89 · 2024-02-22T08:09:26Z

lib/trento_web/views/v1/sap_system_view.ex

+    instance
+    |> Map.from_struct()
+    |> Map.delete(:__meta__)
+    |> Map.delete(:absent_at)


We shouldn't remove this field. It is used in the frontend.
The error must be that it is not defined in the schema.

arbulu89 · 2024-02-22T08:10:31Z

lib/trento_web/views/v1/sap_system_view.ex

+  end
+
+  def render("broadcast_database.json", %{
+        database:


I think we could render inside this the database.json view.
After all, they have the exact same fields, both the database and instance (removing the deregistered_at is not a big deal, we don't use it in the frontend).
Anyway, if having it explicit makes sense to you, all good from my side, but they should have the same content

arbulu89 · 2024-02-22T08:11:50Z

lib/trento_web/views/v1/sap_system_view.ex

+    instance
+    |> Map.from_struct()
+    |> Map.delete(:__meta__)
+    |> Map.delete(:absent_at)


We cannot remove the absent_at, it is used in the frontend.
And it would look like exactly as the broadcast

arbulu89 · 2024-02-22T08:16:39Z

lib/trento_web/views/v1/sap_system_view.ex

+    |> add_system_replication_status_to_secondary_instance
+  end
+
+  def render("broadcast_sap_system.json", %{


You can remove the deregistered_at from the broadcast as well

arbulu89 · 2024-02-22T08:23:43Z

lib/trento_web/views/v2/cluster_view.ex

    |> Map.delete(:__meta__)
  end

+  def render("cluster_details.json", %{details: %{"sap_systems" => sap_systems} = details}) do
+    sap_systems =
+      Enum.map(sap_systems, &Map.take(&1, [:distributed, :filesystem_resource_based, :nodes]))


What is this for? to remove the sid field?
The sid cannot be removed, it is used in the frontend.
Yet again, most probably the schema is what it is wrong

CDimonaco · 2024-02-22T08:33:51Z

I took schema and tests for granted 👀

So, back to draft and we need to start back from schemas, good catch, fixing this things will give more confidence, but it's a real mess, we will learn a very valuable lesson in the end, I hope so 👀

Anyway some code duplication is completely on purpose, and if we want to avoid we need to rethink all the schemas because rn broadcasted info and controller one are very different

This reverts commit 8869575.

arbulu89

Now it looks great @CDimonaco
+1000

This reverts commit 2df1abd.

CDimonaco added enhancement New feature or request tech debt elixir Pull requests that update Elixir code labels Feb 21, 2024

CDimonaco requested a review from arbulu89 February 21, 2024 12:07

CDimonaco self-assigned this Feb 21, 2024

CDimonaco force-pushed the openapi_schema_no_additional_properties branch from f89e28f to f0206af Compare February 21, 2024 12:11

CDimonaco requested a review from dottorblaster February 21, 2024 12:11

CDimonaco added 7 commits February 21, 2024 13:12

Add additionalProperties false to openapi schema objects

2a1d0cb

Cluster v1/v2 controller tests and view refactored for openapi

b386aa9

compatibility

HealthOverview controller refactored for openapi compatibility

8869575

Host views aligned with openapi spex

9532571

Sap system view adapted to sap system controller openapi

f0206af

Fixed Host schema v1

9b2c150

mix credo fix

46cfc67

dottorblaster approved these changes Feb 21, 2024

View reviewed changes

arbulu89 requested changes Feb 22, 2024

View reviewed changes

CDimonaco marked this pull request as draft February 22, 2024 08:47

Revert "HealthOverview controller refactored for openapi compatibility"

3cfdb3f

This reverts commit 8869575.

CDimonaco marked this pull request as ready for review February 22, 2024 09:44

Addressing review feedbacks

c30a5e9

CDimonaco force-pushed the openapi_schema_no_additional_properties branch from 19d291e to c30a5e9 Compare February 22, 2024 09:46

arbulu89 approved these changes Feb 22, 2024

View reviewed changes

CDimonaco added 2 commits February 22, 2024 11:03

Temporary fix incompatible api check

2df1abd

Revert "Temporary fix incompatible api check"

4368429

This reverts commit 2df1abd.

CDimonaco merged commit ff64338 into main Feb 22, 2024
23 of 24 checks passed

CDimonaco deleted the openapi_schema_no_additional_properties branch February 22, 2024 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openapi schemas no additional properties #2355

Openapi schemas no additional properties #2355

CDimonaco commented Feb 21, 2024

dottorblaster left a comment

arbulu89 left a comment

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

dottorblaster Feb 22, 2024

nelsonkopliku Feb 22, 2024 •

edited

Loading

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

arbulu89 Feb 22, 2024

CDimonaco commented Feb 22, 2024

arbulu89 left a comment

Openapi schemas no additional properties #2355

Openapi schemas no additional properties #2355

Conversation

CDimonaco commented Feb 21, 2024

Description

dottorblaster left a comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonkopliku Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDimonaco commented Feb 22, 2024

arbulu89 left a comment

Choose a reason for hiding this comment

nelsonkopliku Feb 22, 2024 •

edited

Loading