node-casbin / sequelize-adapter

Sequelize adapter for Casbin
https://github.com/casbin/node-casbin
Apache License 2.0
64 stars 34 forks source link

Bug in table schema #31

Closed alikhan866 closed 4 years ago

alikhan866 commented 4 years ago

So i ran this in java

enforcer.addPolicy("abcd","123","read");

This creates a table casbin_rule which is fine with columns

ptype,v0,v1,v2,v3,v4,v5

But now i connect the same database with NodeJS with casbin-sequelize-adapter

const casbin = require('casbin');
const { SequelizeAdapter } = require('casbin-sequelize-adapter');
(async () => {
    const a = await SequelizeAdapter.newAdapter({
        host: 'localhost',
        port: '3306',
        username: 'root',
        password: 'root',
        database: 'myhiber',
        dialect: 'mysql',
    });

    const e = await casbin.newEnforcer('../model.conf', a);

    // Check the permission.
    console.log(e.enforce('abcd', '123', 'read'))
})()

But it is throwing an error

Executing (default): SELECT 1+1 AS result
Executing (default): CREATE TABLE IF NOT EXISTS `casbin_rule` (`id` INTEGER NOT NULL auto_increment , `ptype` VARCHAR(255), `v0` VARCHAR(255), `v1` VARCHAR(255), `v2` VARCHAR(255), `v3` VARCHAR(255), `v4` VARCHAR(255), `v5` VARCHAR(255), PRIMARY KEY (`id`)) ENGINE=InnoDB;
Executing (default): SHOW INDEX FROM `casbin_rule` FROM `myhiber`
Executing (default): SELECT `id`, `ptype`, `v0`, `v1`, `v2`, `v3`, `v4`, `v5` FROM `casbin_rule` AS `CasbinRule`;
(node:6328) UnhandledPromiseRejectionWarning: SequelizeDatabaseError: Unknown column 'id' in 'field list'

Why is it trying to select a different schema structure in default compared to Java ? hence the error log ..

Selecting id is the issue here

hsluoyz commented 4 years ago

@tldyl @zhmushan please work together (Java + Node.js) to solve this inconsitency issue.

/cc @nodece @GopherJ

alikhan866 commented 4 years ago

@hsluoyz @nodece @tldyl @zhmushan @GopherJ

Can you please give me an update on this inconsistency issue ?

hsluoyz commented 4 years ago

Hi @alikhan866 , sorry for late response. Which Java adapter did you use?

hsluoyz commented 4 years ago

From your code: https://github.com/alikhan866/casbin-role-based/search?q=adapter&unscoped_q=adapter, I think you are using: https://github.com/jcasbin/jdbc-adapter ?

@tldyl can you add the id field to the JDBC adapter?

alikhan866 commented 4 years ago

@hsluoyz I think id field should be either added across all adapters (which support mysql) or removed across all adapters so we have a schema consistency may it be Node,Go ,Java , Python or any other programming language.. Thanks alot for taking up my issue @hsluoyz

Edit Yes i am using JDBC Adapter

nodece commented 4 years ago

@alikhan866 If you use casbin in multiple programming languages and use the same table, I recommend that you create your own adapter. We can't sure that every adapter is the same.

alikhan866 commented 4 years ago

@nodece i understand there are a LOT of adapters and making them consistent is hard/cumbersome .. but i think there should be atleast 1 adapter that is cross compatible for now :)

@hsluoyz @nodece should i expect my issue to be resolved ? or do i have to write a custom adapter for Java and Node to overcome this inconsistency issue please let me know

hsluoyz commented 4 years ago

@alikhan866 please review this PR.

alikhan866 commented 4 years ago

@hsluoyz ok

alikhan866 commented 4 years ago

@hsluoyz i approved the PR looks good

hsluoyz commented 4 years ago

@ali have you tried the code?

alikhan866 commented 4 years ago

@hsluoyz Give me some time I'll try and comment on the PR accordingly

hsluoyz commented 4 years ago

@alikhan866 please review this PR instead: https://github.com/jcasbin/jdbc-adapter/pull/20

alikhan866 commented 4 years ago

@hsluoyz I just ran the code and i have verified that id field is getting added ..

nodece commented 4 years ago

@alikhan866 https://github.com/jcasbin/jdbc-adapter/pull/20 looks good, but we should be note that id value. currently, the id value base on policy size, if we are running multiple casbin in multiple server, the id is incorrect, I added comment to https://github.com/jcasbin/jdbc-adapter/pull/20.

alikhan866 commented 4 years ago

@nodece Actually my use-case of casbin is that i need to integrate it with an existing java-backend so over there i want to just use e.enforce to enforce the existing policies in the db to the backend

And i need to create a seperate back-end and front-end for Role management to manipulate those roles easily so.. i chose Express (NodeJS) + React .. I know casbin has a web dashboard but i need to develop a custom one that is more user friendly

And i really didnt want to use Java for the back-end because of too much boiler plate code

Your comment is definitely better way to do this i have upvoted it though 👍

hsluoyz commented 4 years ago

@alikhan866 yes, our own web dashboard still needs many improvements. We also welcome 3rd-party web dashboard to be open-sourced, so we can put it in the official site.

alikhan866 commented 4 years ago

@hsluoyz Okay , i will notify you once i complete development of my own dashboard .. btw can you please tell me if i should expect this issue to be merged to master ? or will i have to use the code manually .. thanks once again for resolving my issue Casbin Team

hsluoyz commented 4 years ago

@alikhan866 we are still in process of reviewing the PR: https://github.com/jcasbin/jdbc-adapter/pull/20 . You can also give comment on latest commits to accelerate it.

hsluoyz commented 4 years ago

Update

the PR is merged. New Maven package is on the way.