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

Normal / Admin connection #11

Closed
inzanez opened this issue Apr 22, 2020 · 4 comments
Closed

Normal / Admin connection #11

inzanez opened this issue Apr 22, 2020 · 4 comments

Comments

@inzanez
Copy link
Contributor

inzanez commented Apr 22, 2020

Hello there
I was wondering if there were any specific reason for having two different connection types for Admin and Normal connections? Is this really a requirement, or could we just switch to a single connection type?

I'm justing seeing some problems in r2d2 as the connections cannot be converted into admin connections. And so I started thinking about it and I somehow do not really see any advantage of having this distinction. The ArangoDB API will respond with an according error if someone without access is trying to modify the _system database.

@fMeow
Copy link
Owner

fMeow commented Apr 22, 2020

My purpose is to avoid a dangling Database instance that the database it refers to has been dropped.

Say we have an instance of connection conn, and we derived a database instance db and the name of database is test_db:

let conn = Connection::establish_jwt("http://localhost:8529", "username", "password").await.unwrap();
let db = conn.db("test_db").unwrap();

If we simple drop database test_db with conn, the db object is a nonsense that every request fails on runtime. Since rust is a compile language, it's not like python or javascript that a coder can instantly response in the interpreter. Instead of handling it after it happen, it's better to avoid it in the first place if we know it will fails. In this specific case, I think it is better to fail at compiler time than runtime.

Maybe we can move the create_database to Normal role and only keep drop_database in Admin, this might alleviate your problems with r2d2. Though this can cause confusion.

BTW, the async/await support is on air in develop branch. I will publish it once I finished strong typing lib error.

@inzanez
Copy link
Contributor Author

inzanez commented Apr 22, 2020

@fMeow Thank you for your answer. I somehow don't quite agree. The part about Rust being a statically typed language with many compiler checks refers mainly to the fact that unsafe programming styles should be mitigated, like ending up with dangling pointers, not handling paths in a decision tree etc. So it removes a lot of possibilities for fatal crashes / vulnerabilities / other unwanted effects.

Regarding the database connection, I agree that a connection to a deleted database will not be of any value. But handling an error of this form (a database that's being queried does not exist anymore) is not something you can possibly verify at compile time. For one part, it could very well be that the ArangoDB cluster houses other applications with admin access (and they could for whatever reason delete a database still in use), or an administrator could do the same. I could actually even do the same with the current 'arangors' implementation by spawning a separate admin connection and deleting the database.

Apart from that I also think that the error is quite nice to handle, as ArangoDB replies:

{
  "error": true,
  "errorNum": 1228,
  "errorMessage": "database not found",
  "code": 404
}

So pushing everything into one connection could make sense, as it will remove some complexity that might not be required. What do you think?

BTW, the async/await support is on air in develop branch. I will publish it once I finished strong typing lib error.

That's very pleasant news!

@fMeow
Copy link
Owner

fMeow commented Apr 23, 2020

Thank you for your illustration.

I make it clear that Admin role is the one to perform admin operation, like something on _system database, backup/restore and other functions that can be reached in _api/admin.

Certainly creation and delete of database should not counted as admin role. So I moved create_database and drop_database to Normal role.

This comes in the 0.3 version.

@inzanez
Copy link
Contributor Author

inzanez commented Apr 23, 2020

@fMeow perfect, I'm looking forward to those changes. Many thanks!

@inzanez inzanez closed this as completed Apr 23, 2020
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

No branches or pull requests

2 participants