Add ArangoDB to rocket_contrib databases

[Weasy666]

This PR adds support for ArangoDB by implementing Poolable for arangors::Connection in conjunction with r2d2_arangors.

arangors expects a url, username and password for its Connection::establish_...() functions, so i had to make it mandatory, to include them into the Rocket.toml file. I hope that is ok, if not...another idea could be to use a http://root:root@localhost:8529 url and extract the user and password from it.

[inzanez]

@Weasy666 wouldn't make sense to integrate using mobc-arangors as this would provide an async connection pool including async database driver connection? At least if that should end up in 0.5 / async version of Rocket.

[Weasy666]

I went with r2d2-arangors, because r2d2 is already used as pool for the other databases and i am not sure if @SergioBenitez and @jebrosen would want to integrate a different one.

[inzanez]

@Weasy666 Oh, that makes sense I guess. Well, I will wait what happens on that end. If required, I will upgrade r2d2-arangors to allow serving async driver connections.

[jebrosen]

The database pooling support implemented in rocket_contrib in the master branch is for synchronous databases, with a wrapper for using them safely in async code (#1375); this is partly because several of the existing supported drivers from 0.4 have no good async replacements. There is no async database pool support yet; there's some more discussion of that in #1117.

To be usable with the current (synchronous) implementation of #[database], this PR would need to use r2d2_arangors and the blocking mode of arangors - which it looks like it does.

One concern I have with this approach is that it could make it difficult to add an integration with the async version of arangors until this one is removed (i.e. in a new major version of rocket_contrib). This is because you can't use the async and sync versions of the arangors crate simultaneously within one workspace, so we would need to clearly document that and make sure nothing (including e.g. rocket's own CI) ends up in that position.

[inzanez]

@jebrosen couldn't both be integrated using features? Sync would be using r2d2_arangors which should work even if rocket is async, async would be using mobc or whatever will find it's way into async rocket.

[jebrosen]

@inzanez Feature flags for dependencies are combined, so arangors will either be async or not-async for the whole project. If the synchronous version is used (e.g. for r2d2-arangors), then any code that uses the async API for arangors (such as mobc-arangors) won't compile. This would be a problem if rocket_contrib wants to add an async integration later, because you would have to make sure to never try to compile both at the same time.

[inzanez]

@jebrosen I see. That makes totally sense. Maybe it would be better to wait for version 0.5 to be released and integrate the async version? In the meantime, @Weasy666 could be using r2d2-arangors with the pool as managed state? I currently have it integrated in a struct called AppConfig that is contained in an Arc. It carries application configuration data (non-rocket related) and the database pool from r2d2. Works pretty well, happy to share some code.

[Weasy666]

@inzanez thanks for the offer. I'm already working with my fork and i think will continue doing that until one of the different options finds its way inside rocket_contrib. I'll come back to your offer, when i'm getting too annoyed with rebasing to keep up with Rocket.

@jebrosen That is a really annoying "feature" of cargo, with which i fought a few time in the past. I can understand your concerns about this problem, but doesn't this also already apply to the currently integrated databases? Or is arangors somewhat special in this regard because of its async and blocking features?

[jebrosen]

@Weasy666 This situation is unique to arangors at least compared to rocket_contrib's existing database integrations, because it has feature flags that change code instead of adding it. The existing databases provide only blocking APIs, sometimes with an async API in an entirely separate crate. Another approach that does not have this issue is to provide both async and blocking APIs simultaneously but in different modules, similarly to reqwest vs reqwest::blocking.

I personally don't have any huge issues with integrating arangors as either async or blocking; I only want to avoid the situation where we want to add it as async later and discover we can't.

[Weasy666]

Ok, that makes sense...then i will try and ask the arangors maintainer, about their opinion to maybe go the reqwest way and split up sync and async in a breaking change.

[SergioBenitez]

Indeed, it looks like the arangors project is misusing feature flags. This needs to be resolved before this PR can be merged.

[SergioBenitez]

Where are these configuration options documented? We'll need them somewhere, otherwise it'll be impossible for someone to properly configure this database and thus use it at all.

[Weasy666]

That's right, i will write something about that. Is it ok to add it here, right above the ## Extended?
https://github.com/SergioBenitez/Rocket/blob/77faddfc8b458f5b23135f86fa857573cc18d81c/contrib/lib/src/databases.rs#L352-L353

[Weasy666]

I've added docs at that position. Feel free to move them around if that is the wrong place 😅

[SergioBenitez]

Marking as a draft until we have upstream movement.

[SergioBenitez]

I am closing this given that there is no progress possible until upstream resolves this issue. Furthermore, we will start advocating for using the async database pools in #1696 as soon as it's ready, eventually deprecating sync_db_pools. If there is an async ArangoDB driver, we should seek to integrate it into #1696.

[Weasy666]

@SergioBenitez @jebrosen
Understandable, seeing the lack of progress/activity upstream.
arangors is not only a sync but also an async driver, which was the whole problem here, or better its handling of both features. If it is not a problem for you that, when integrating async arangors, a user can not use sync arangors in rocket while using the rocket provided async integration of arangors, then i can take a look and try to integrate it after #1696 was merged.

[SergioBenitez]

I think that would be a better, if imperfect, compromise.