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

Easier debugging of detected relationships #3752

Open
steve-chavez opened this issue Oct 21, 2024 · 4 comments
Open

Easier debugging of detected relationships #3752

steve-chavez opened this issue Oct 21, 2024 · 4 comments
Labels
idea Needs of discussion to become an enhancement, not ready for implementation

Comments

@steve-chavez
Copy link
Member

steve-chavez commented Oct 21, 2024

Problem

Let's say I want to debug if there's a one-to-one relationship detected (sometimes this can be hard in the presence of views).

Using the runtime schema cache and jq it can be done by searching the table name (country) and the O2O tag:

# start postgrest
# PGRST_ADMIN_SERVER_PORT=3001 PGRST_DB_AGGREGATES_ENABLED=true postgrest-with-postgresql-16  -f test/spec/fixtures/load.sql postgrest-run

curl 'localhost:3001/schema_cache' | \
jq '.dbRelationships | .. | objects | select(.relTable.qiName == "country" and .relCardinality.tag == "O2O")'

{
  "relCardinality": {
    "isParent": true,
    "relColumns": [
      [
        "id",
        "country_id"
      ]
    ],
    "relCons": "capital_country_id_fkey",
    "tag": "O2O"
  },
  "relFTableIsView": false,
  "relForeignTable": {
    "qiName": "capital",
    "qiSchema": "test"
  },
  "relIsSelf": false,
  "relTable": {
    "qiName": "country",
    "qiSchema": "test"
  },
  "relTableIsView": false,
  "tag": "Relationship"
}

This has some issues:

  • Not straightforward and requires another tool for filtering.
  • The Admin API has to be involved, which is internal and there's no reason why this information cannot be public facing. -
  • We expose OpenAPI to the public but it doesn't have this information.

Solution

A custom media type that shows all the relationships for a resource.

GET /country
Accept: application/vnd.pgrst.relationships+json
[
    {
      "relCardinality": {
        "isParent": true,
        "relColumns": [
          [
            "id",
            "country_id"
          ]
        ],
        "relCons": "capital_country_id_fkey",
        "tag": "O2O"
      },
      "relFTableIsView": false,
      "relForeignTable": {
        "qiName": "capital",
        "qiSchema": "test"
      },
      "relIsSelf": false,
      "relTable": {
        "qiName": "country",
        "qiSchema": "test"
      },
      "relTableIsView": false,
      "tag": "Relationship"
    }
]

Related

@steve-chavez steve-chavez added the idea Needs of discussion to become an enhancement, not ready for implementation label Oct 21, 2024
@wolfgangwalther
Copy link
Member

Hm, some thoughts:

  • Wouldn't it be much better to represent those relationships properly inside the OpenAPI output instead of with a entirely separate result?
  • Just changing the accepted media type should not suddenly return metadata instead of actual data on the same endpoint. The response should be represented differently, but the actual content should be the same. Not the case in your proposal.
  • We should not dump the internal types as-is, if we do something like that. Otherwise every change to those internal types will be a breaking change to our api. The admin server's schema_cache endpoint is already borderline - we should probably document that the actual response format is not safe to rely on across any version (not even patch), because it might change at any time.

@steve-chavez
Copy link
Member Author

steve-chavez commented Oct 24, 2024

Wouldn't it be much better to represent those relationships properly inside the OpenAPI output instead of with a entirely separate result?

@laurenceisla Since you've been working on OpenAPI, do you know if there's a way to represent this metadata? (whether openAPI v2 or v3)

Just changing the accepted media type should not suddenly return metadata instead of actual data on the same endpoint. The response should be represented differently, but the actual content should be the same. Not the case in your proposal.

But we do that for the the explain plan right? Overall I thought that is much more convenient to do GET /tbl + header than GET /?resource=tbl on the root endpoint, but I wouldn't be opposed to doing the latter. Specially if we add some response header that links to the resource definition when doing GET /tbl. I recall that there was a standard header about that.

We should not dump the internal types as-is, if we do something like that. Otherwise every change to those internal types will be a breaking change to our api. The admin server's schema_cache endpoint is already borderline - we should probably document that the actual response format is not safe to rely on across any version (not even patch), because it might change at any time.

Agree.

@laurenceisla
Copy link
Member

laurenceisla commented Oct 25, 2024

@laurenceisla Since you've been working on OpenAPI, do you know if there's a way to represent this metadata? (whether openAPI v2 or v3)

One way would be to include the embedding in the schema for the content-type of Response Objects. For example, the schema could look something like:

{
  "schema": {
    "type": "array",
    "items": {
      "type": "object",
      "required": [
        "id",
        "name"
      ],
      "properties": {
        "id": {"..."},
        "name": {"..."},
        // Show the relationship here, as another property of the response object
        "child_entities": {
          "type": "object",
          "properties": {"..."}
        }
      }
    }
  }
}

Since child_entities is an "object", not an "array", then it's a to-one relationship. To check if it's a one-to-one, the schema for the /child_entities response would need to be checked as well.

Edit: If I'm not mistaken a description can also be added to the child_entities property and, additionally, mention the cardinality there.

@wolfgangwalther
Copy link
Member

But we do that for the the explain plan right? Overall I thought that is much more convenient to do GET /tbl + header than GET /?resource=tbl on the root endpoint, but I wouldn't be opposed to doing the latter. Specially if we add some response header that links to the resource definition when doing GET /tbl. I recall that there was a standard header about that.

Right, we do. But the situation is different. The "query to fetch this data" is much more connected to the "data" than the relationship metadata proposed here. Think about it differently: If you add various filters, select, limit etc. to the request's URL - those would not make sense at all for the proposal here. But they do make a lot of sense for the explain output.

@PostgREST PostgREST deleted a comment Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Needs of discussion to become an enhancement, not ready for implementation
Development

No branches or pull requests

3 participants