search

cychiuae / casbin-pg-adapter

PostgreSQL adapter for Casbin
Apache License 2.0
9 stars 6 forks source link

Add PK or set replication identity #4

Open fgblomqvist opened 3 years ago

fgblomqvist commented 3 years ago

Since the table that is created does not have a PK, it's impossible to do DELETE/UPDATE statements on it in case the DB instance has a publication set for ALL TABLES (https://www.postgresql.org/docs/current/logical-replication-publication.html). This is not uncommon in more complex environments.

In order to prevent people from having to fix the table created by this adapter, I would propose either adding a PK to it or setting the REPLICA IDENTITY to FULL. If you go with the PK, you could either do it on the full row (not sure if there are any bad side effects from that) or simply add an ID column (seems a bit useless perhaps). Setting the REPLICA IDENTITY can be done using ALTER TABLE ... REPLICA IDENTITY FULL.

hsluoyz commented 3 years ago

@fgblomqvist we should add a unique ID column like how we did for Gorm Adapter: https://github.com/casbin/gorm-adapter/pull/54

fgblomqvist commented 3 years ago

One question I got is since Casbin wants adapters to fully manage their DBs/tables, I'm guessing that means that a migration has to be created for this? How will that work out?

hsluoyz commented 3 years ago

@closetool do you think we need migration?

kilosonc commented 3 years ago

@hsluoyz I don't get it why it's necessary

hsluoyz commented 3 years ago

@fgblomqvist I think we can put a notice about the db format change, like how we did it previously: https://github.com/jcasbin/casbin-spring-boot-starter/issues/25

fgblomqvist commented 3 years ago

It's just a bit uggly to force the user to add the field when Casbin does the table creation. If it was completely managed by the user it would make sense. But oh well.

hsluoyz commented 3 years ago

@fgblomqvist what kind of DB migration do you refer to? Can you provide some details?

fgblomqvist commented 3 years ago

https://www.cloudbees.com/blog/database-migration/ Obviously you wouldn't just "manually add the column" in a production system. You'd need a migration. If you guys add that column without automatically migrating the existing table, it becomes a breaking change. Technically you could easily check when Casbin loads if that column exists, and if it doesn't run the SQL to add it. You could strip that code in a later major version.

hsluoyz commented 3 years ago

@fgblomqvist OK. Then I think we are on the same page. The question on my mind is that is there any production use for this adapter given its popularity? If not, the migration code will become useless efforts.

fgblomqvist commented 3 years ago

Fair enough, perhaps the adoption is not there yet. I can figure things out on my end. But regardless, having a PK column would be very nice indeed.

hsluoyz commented 3 years ago

@fgblomqvist thanks for understanding. Currently still waiting for: https://github.com/cychiuae/casbin-pg-adapter/pull/8 to be merged, then we will fix this issue.