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

Hard to upgrade from v9.0.1 #3290

Closed
steve-chavez opened this issue Feb 27, 2024 · 8 comments · Fixed by #3312
Closed

Hard to upgrade from v9.0.1 #3290

steve-chavez opened this issue Feb 27, 2024 · 8 comments · Fixed by #3312
Labels
idea Needs of discussion to become an enhancement, not ready for implementation

Comments

@steve-chavez
Copy link
Member

Problem

v10.0.0 introduced one-to-one relationship detection #2439, which changed the json output of responses using resource embedding. For upgrading, adding computed relationships was recommended but this is considerable work for projects that have many tables. For this reason some projects are stuck on v9.0.1 .

Proposal

The following config can be used to determine which relationships will be represented as json objects. By default it will be:

db-resource-embedding-single = "many-to-one, one-to-one"

So projects stuck on 9.0.1 can do:

db-resource-embedding-single = "many-to-one"

Related

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

I don't like adding config options for this purpose. We should provide better migration paths, instead: Docs, Tools, anything that helps the actual migration.

For upgrading, adding computed relationships was recommended but this is considerable work for projects that have many tables.

Could this be (partly) automated with dynamic SQL to create the computed relationships?

@steve-chavez
Copy link
Member Author

Yeah, I don't like it either. It also provides an easy way to have incompatible outputs between postgREST APIs, that could cause trouble.

We should provide better migration paths, instead: Docs, Tools, anything that helps the actual migration.
Could this be (partly) automated with dynamic SQL to create the computed relationships?

I was thinking we could have a postgrest --dump-relationships=one-to-one command, its output could help with generating the dynamic SQL.

@wolfgangwalther
Copy link
Member

I was thinking we could have a postgrest --dump-relationships=one-to-one command, its output could help with generating the dynamic SQL.

No need for another CLI flag, we already have everything we need:

postgrest-with-postgresql-16 -f test/spec/fixtures/load.sql postgrest-run --dump-schema \
  | jq '.dbRelationships | .[] | .[1] | .[] | select(.relCardinality.tag == "O2O")'

@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 4, 2024

Just using jq looks like a good idea.

Removing some unnecessary keys:

postgrest-with-postgresql-16 -f test/spec/fixtures/load.sql postgrest-run --dump-schema \
  | jq '.dbRelationships | .[] | .[1] | .[] | select(.relCardinality.tag == "O2O")' \
  | jq 'del(.relFTableIsView,.relTableIsView,.tag,.relIsSelf)'

We get an output like:

{
  "relCardinality": {
    "relColumns": [
      [
        "id",
        "id"
      ],
      [
        "code",
        "code"
      ]
    ],
    "relCons": "students_info_id_code_fkey",
    "tag": "O2O"
  },
  "relForeignTable": {
    "qiName": "students",
    "qiSchema": "test"
  },
  "relTable": {
    "qiName": "students_info",
    "qiSchema": "test"
  }
}

{
  "relCardinality": {
    "relColumns": [
      [
        "id",
        "id"
      ],
      [
        "code",
        "code"
      ]
    ],
    "relCons": "students_info_id_code_fkey",
    "tag": "O2O"
  },
  "relForeignTable": {
    "qiName": "students_info",
    "qiSchema": "test"
  },
  "relTable": {
    "qiName": "students",
    "qiSchema": "test"
  }
}

The relevant information is there though it's cumbersome with a lot of o2o. And we need to define computed rels for both ends..

Next steps:

  • Create a bash script to make this easier. It should parse the json output and then generate the SQL statements.
  • Include the bash script on the v9.0.1 release artifacts.

Edit: jq expression that aggregates the objects into an array

postgrest-with-postgresql-16 -f test/spec/fixtures/load.sql postgrest-run --dump-schema  \
| jq  '[.dbRelationships | .[] | .[1] | .[] | select(.relCardinality.tag == "O2O" and .relFTableIsView == false and .relTableIsView == false) | del(.relFTableIsView,.relTableIsView,.tag,.relIsSelf)]'

@wolfgangwalther
Copy link
Member

Include the bash script on the v9.0.1 release artifacts.

Surely you mean the v10 release artifacts, because that's where the breaking changes were introduced, right?

@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 5, 2024

We need to create two computed rels, an o2m and a m2o for correcting the o2o rel. So the resulting computed rels from the students/students_info example shown above should be:

-- students -> students_info o2m
create function students_info(students) returns setof students_info rows 1000 as $$
  select * from students_info where id = $1.id and code = $1.code
$$ language sql;

-- students_info -> students m2o
create function students(students_info) returns setof students rows 1 as $$
  select * from students where id = $1.id and code = $1.code
$$ language sql;

But there's a problem with the schema cache json output. It doesn't show on which table is the foreign key defined. So we cannot know to which table the "many" end belongs to.

So the current output is insufficient for having this automated 😞.


Maybe the problem is that we add the relationship and the inverse relationship. If we only had one then this should be solvable.

Edit: the simplest solution would be to add an attribute to Cardinality. But then users would need to use the next v12 version for the migration script..

@laurenceisla
Copy link
Member

It doesn't show on which table is the foreign key defined

I was having a similar issue when working on #3226, since we need to know to which table to insert first in a o2o relationship.

the simplest solution would be to add an attribute to Cardinality.

So I think this solution would be necessary anyways.

@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 7, 2024

For this issue it doesn't seem worth it changing the Relationship type. Will close with #3312.

So I think this solution would be necessary anyways.

I agree. That could be a refactor:.. PR.

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

Successfully merging a pull request may close this issue.

3 participants