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

Behaviour of NewAdapterByDB() has a breaking change #78

Closed
d1ss0nanz opened this issue Feb 22, 2021 · 17 comments · Fixed by #79
Closed

Behaviour of NewAdapterByDB() has a breaking change #78

d1ss0nanz opened this issue Feb 22, 2021 · 17 comments · Fixed by #79
Assignees
Labels

Comments

@d1ss0nanz
Copy link

d1ss0nanz commented Feb 22, 2021

Hi,

Just a heads up for other users:
I've updated from gorm-adapter 3.0.3 to 3.2.0 today and found that NewAdapterByDB() is now using "casbin_rule" as table name vs. "casbin_rules" in 3.0.3.

Therefore the user group memberships where missing in the new "casbin_rule" table.

The reason is probably (didn't verify) the use of the Gorm Migrator in 3.0.3:
return a.db.Migrator().CreateTable(a.getTableInstance())
In Gorm "CasbinRule" will result in a table name "casbin_rules".

This fixed if for me:
insert into casbin_rule(ptype,v0,v1) select p_type,v0,v1 from casbin_rules where p_type = 'g';

@hsluoyz
Copy link
Member

hsluoyz commented Feb 22, 2021

@d1ss0nanz I think casbin_rule has always been the rule name? Even before v3.0.3, we have:

image

@d1ss0nanz
Copy link
Author

Yes, but that's not the cause. I've checked my old logs and my database initialisation scripts.
It was "casbin_rules" in 3.0.3.

Please see my note about the use of the Migrator.

@hsluoyz
Copy link
Member

hsluoyz commented Feb 22, 2021

The reason is probably (didn't verify) the use of the Gorm Migrator in 3.0.3:
return a.db.Migrator().CreateTable(a.getTableInstance())
In Gorm "CasbinRule" will result in a table name "casbin_rules".

@closetool is it possible to fix this bug?

@hsluoyz hsluoyz self-assigned this Feb 22, 2021
@hsluoyz hsluoyz added the bug label Feb 22, 2021
@kilosonc
Copy link
Contributor

In v3.0.3 NewAdapterByDB will create table named casbin_rules, because of this
image
And NewAdapter will create table named casbin_rules

In v3.2.0 they both create table named casbin_rule,
I think it works fine now, nothing needed to change.

@kilosonc
Copy link
Contributor

@d1ss0nanz What can I do for you?

@hsluoyz
Copy link
Member

hsluoyz commented Feb 23, 2021

@closetool if someone is running our migration code (the following line):

return a.db.Migrator().CreateTable(a.getTableInstance())

Will they end up with the wrong table name: casbin_rules ?

@kilosonc
Copy link
Contributor

kilosonc commented Feb 23, 2021

@hsluoyz In v3.2.0, there will be no casbin_rules default, in spite of that, I changed the CasbinRule's name mapping

@d1ss0nanz
Copy link
Author

Ok, so up to at least 3.0.3, in some code paths, the created table is named "casbin_rules".
NewAdapterByDB() is behaving this way in 3.0.3.

Now, when updating to a more recent version, gorm-adapter is creating a new table "casbin_rule" besides the fact that there already exists a table "casbin_rules".

All authorization attempts silently fail, since the group membership data is in the old table.

I would suggest that you document the breaking change, so one doesn't have to understand how Gorm derives table names from struct names, to understand what's going on.

Optionally, there should be migration code that checks for a table "casbin_rules" and moves the data over when creating "casbin_rule".

@00LT00
Copy link
Member

00LT00 commented Mar 10, 2021

@hsluoyz @closetool Hello, I think it was a new bug with new version gorm.
We use the scopes function to realize the dynamic table name. like this

        a.db = db.Scopes(a.casbinRuleTable()).Session(&gorm.Session{})
	err := a.createTable()

It was successful work at least version 1.20.8.
But now, even if we use this version too, new users also will use new version gorm, which make us have to use new version to run. I'm already asking the author this bug at here. I think it's necessary to remind users that not use the latest gorm version in their projects on readme.md before the author has a new reply.

@kilosonc
Copy link
Contributor

@00LT00 @hsluoyz I noticed we have run up some commits from last releasing new version
In #79, I added default table name function, and I believe it will work even if scope failed.

@00LT00
Copy link
Member

00LT00 commented Mar 10, 2021

@closetool But I get the same error, why?
If I update the version of gorm on go.mod, it was work. I think it was a bug of gorm rather than ours

@d1ss0nanz
Copy link
Author

@00LT00 For me this was caused by using NewAdapterByDB(db) and from looking at the code I assume that NewAdapterByDBUseTableName(db, "", "casbin_rules") should do the trick.

But I've decided to just migrate the data into "casbin_rule" and go with the default.

@00LT00
Copy link
Member

00LT00 commented Mar 10, 2021

@00LT00 For me this was caused by using NewAdapterByDB(db) and from looking at the code I assume that NewAdapterByDBUseTableName(db, "", "casbin_rules") should do the trick.

But I've decided to just migrate the data into "casbin_rule" and go with the default.

It just fix your error, but the bug is there.

@00LT00
Copy link
Member

00LT00 commented Mar 10, 2021

hi @closetool I know why....
The latest version 3.2.0 did not import your changes. So we should release a new version for #79 ... @hsluoyz

@kilosonc
Copy link
Contributor

@00LT00 yep

@00LT00
Copy link
Member

00LT00 commented Mar 10, 2021

@closetool But it just fix the defalt table names. If user use dynamic table name, and the table is not exist. Then the table will not be create.
image
So I still suggest we should remind users not to update version of gorm more than 1.20.12.

@hsluoyz
Copy link
Member

hsluoyz commented Mar 14, 2021

@00LT00 thanks! I saw Jinzhu has fixed that bug. So updating to Gorm master HEAD will fix the issue.

@d1ss0nanz Bug fixed in: https://github.com/casbin/gorm-adapter/releases/tag/v3.2.2

@hsluoyz hsluoyz closed this as completed Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants