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

feat: allow multiple connections #33

Closed
wants to merge 3 commits into from

Conversation

fdionisi
Copy link
Contributor

This PR allows casbin-mongoose-adapter to instantiate multiple
connections to different connections strings. It makes it easier to test
code that depends on this adapter and give more flexibility to where casbin
policies will be stored.

This PR allows `casbin-mongoose-adapter` to instantiate multiple
connections to different connections strings. It makes it easier to test
code that depends on this adapter and give more flexibility to where
casbin policies will be stored.
Comment on lines +34 to +44
it('Should properly instantiate multiple adapters', async () => {
const adapter1 = new MongooseAdapter('mongodb://localhost:27001/casbin', { ...MONGOOSE_OPTIONS, autoCreate: true });
const adapter2 = new MongooseAdapter('mongodb://localhost:27001/casbin2', { ...MONGOOSE_OPTIONS, autoCreate: true });

assert.instanceOf(adapter1, MongooseAdapter);
assert.isFalse(adapter1.isFiltered());

assert.instanceOf(adapter2, MongooseAdapter);
assert.isFalse(adapter2.isFiltered());
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add a test here, but I noticed some others were failing, so not sure if I did this the right way.

If you can direct me, I'd be happy to implement proper tests.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 22, 2021

@fdionisi plz fix:

image

@fdionisi
Copy link
Contributor Author

@hsluoyz I'll do my best here.

When I pull the master branch and run yarn tests, I get the same error as in the CI:
Artboard

It looks something is off with the integration tests (note I added myself a unit test here).

@hsluoyz hsluoyz requested review from Zxilly, Sefriol and ghaiklor and removed request for Zxilly and Sefriol April 22, 2021 16:23
@hsluoyz
Copy link
Member

hsluoyz commented Apr 22, 2021

@Zxilly @Sefriol

@Zxilly
Copy link
Contributor

Zxilly commented Apr 22, 2021

@Zxilly
Copy link
Contributor

Zxilly commented Apr 22, 2021

If you truly want to use such an implement, CasbinRule will be abandoned.

@fdionisi
Copy link
Contributor Author

Oh, now I see what is happening here. The tests are actually getting the transpiled JavaScript code (from lib/ not src/), not TypeScript - totally my bad here. My apology.

@fdionisi
Copy link
Contributor Author

If you truly want to use such an implement, CasbinRule will be abandoned.

@Zxilly Uhm, not sure what you are referring to, but let me dig a bit deeper in the code, and I'll try to get back here with a solution.

@Zxilly
Copy link
Contributor

Zxilly commented Apr 22, 2021

@fdionisi Please read the link I provided above.

@Sefriol
Copy link
Contributor

Sefriol commented Apr 22, 2021

Can somebody explain the use case? The flexibility part especially does not seem very practical or useful to me at least.

@fdionisi
Copy link
Contributor Author

Can somebody explain the use case? The flexibility part especially does not seem very practical or useful to me at least.

Connecting to one MongoDB URL for resources and connect to a different one for casbin rules.
I am trying to test the implementation without compromising my dev database (even if local).

It also shows up running tests in parallel - for some of them, I need a brand new database.

If you believe it doesn't add value, then fair enough - possibly it is useful only to me.

@Sefriol
Copy link
Contributor

Sefriol commented Apr 22, 2021

For initial setup and testing that might be useful temporarily. However I would personally use fixtures, backups or scripts so that you are easily able to reset your testing database to desired state.

Multiple database connection can cause more issues than it tries to solve and in the end I would assume that you want to use singular database for all queries.

Especially when we talk about production environments with clusters and replicas involved.

@fdionisi
Copy link
Contributor Author

@Sefriol, I get it.

The tests are passing correctly. The decision is up to you guys; if you think this brings nothing to the table or the implementation is not to your liking, I understand.

Let me know, and I'll close the PR.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 23, 2021

Sorry that I will close this PR based on above discussions.

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

Successfully merging this pull request may close these issues.

4 participants