casbin / casbin-pg-adapter

A go-pg adapter for casbin
https://github.com/casbin/casbin
Apache License 2.0
38 stars 28 forks source link

Thoughts on using database/sql instead of go-pg? #16

Closed fgblomqvist closed 3 years ago

fgblomqvist commented 3 years ago

Since this adapter is very light-weight, have you guys considered switching out go-pg to the built-in database/sql package? It would make this a very attractive pure SQL adapter with very few/small dependencies.

pckhoi commented 3 years ago

Can you tell us the reason why to switch to database/sql? I didn't stumble on and use go-pg by accident but for its ease of use and practicality.

hsluoyz commented 3 years ago

@fgblomqvist why not use https://github.com/casbin/gorm-adapter or https://github.com/casbin/xorm-adapter ?

fgblomqvist commented 3 years ago

@pckhoi The reason would be because all that is being done are simple "select where" queries as well as basic insert/delete ones. You're basically using the table as a CSV, just storing a bunch of strings. Swapping to database/sql would probably barely result in any more code needing to be written. But hey, it's just a thought. I guess the question is, why do you feel like an ORM is needed?

@hsluoyz Because those also use ORMs (and have several dependencies)?

This is a good example: https://github.com/cychiuae/casbin-pg-adapter

However, it doesn't support filtered policy-loading. But yeah I figured I'd ask here since I was curious what the motivation was for using an ORM, since this is the "official" pg adapter.

pckhoi commented 3 years ago

The ORM choice isn't made specifically to write this adapter. Instead this adapter was made for those who are already using go-pg. Does that answer your question?

hsluoyz commented 3 years ago

@fgblomqvist we have a lot of adapters that support PostgreSQL: https://casbin.org/docs/en/adapters What about these ones?

(the tag "Archived" doesn't matter as we can take it back any time:))

fgblomqvist commented 3 years ago

@pckhoi Yes it does. However, might want to change how this adapter is listed on the adapters page then. It's currently (in that case) incorrectly listed as a "Filtered PostgreSQL adapter" and type "SQL" rather than "go-pg Adapter" type "ORM".

@hsluoyz The first uses sqlx and the latter 2 (which are the same) look unfinished. Either way, like I said, there is a good one already available that uses only database/sql, but it doesn't supported the filtering yet. Probably fairly trivial to add that to it though so I think I'll go that route.

hsluoyz commented 3 years ago

@fgblomqvist OK. Can you also send an issue to: https://github.com/cychiuae/casbin-pg-adapter ? So we can know whether the owner is still active and will make changes for you or be able to review/accept our PR.

fgblomqvist commented 3 years ago

https://github.com/cychiuae/casbin-pg-adapter/issues/3

hsluoyz commented 3 years ago

Closed here.