-
Notifications
You must be signed in to change notification settings - Fork 51
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
ODBC Support #314
Comments
I think this could be solved by features in rust? For server I wouldn't mind too much, but for mobile it could get quite heavy adding these other dependencies. But I'm not a rust expert, so probably @andrewwhitehead and @berendsliedrecht should chime in here |
Unfortunately there are no native rust implementations for ODBC (or for DB2/Oracle/MSSQL/etc) and so I don't see how it could be solved by features in rust. I'm definitely happy to be proven wrong. Thanks. |
I think what @TimoGlastra means is that guard the ODBC functionality behind Rust features. This will allow for the mobile apps to not include this in their build, but it can be enabled for server applications. IMO I am not opposed to the addition, but also not really involved enough in the storage side of askar to know the complexities. |
Yes it should be possible to add it behind a flag in the askar-storage crate. I don't think it would make sense to include it in (for example) the Python build for non-Windows platforms if it's going to require an external library, but it looks like support should always be available on Windows. This would probably be implemented as a single database backend and selected with an "odbc://..." database URL scheme. |
Thanks for the response. I've included some following up questions/comments below:
This is what I was most concerned about - the new library dependency. I really need this capability for the JavaScript wrapper on Linux. Would it be acceptable to add this dependency for X86_64 Linux? If not, I could think of a couple of possible options:
What do you think?
|
@scottexton / @andrewwhitehead (and others) — would it be helpful to have a meeting on the design, or are the two of comfortable with the direction. To be clear, @scottexton — we’d love the contribution, and appreciate the approach of getting the right design up front. As you can tell, I’m not the developer here, so I can facilitate but can’t help with the technical details. |
@swcurran It might be best to wait for Andrew to respond to my latest comment first. This will provide me with a better idea of the approach. After we have finalised the approach I am happy to pull together a description of the proposed changes (aka high level design), and we can see at that point whether a meeting is still required. Thanks. |
@andrewwhitehead Would another option be to include both versions of the compiled library (one with the odbc feature and one without) within the JavaScript package and then selectively load the correct version based on some criteria? |
This would only impact the JavaScript 'server' packages - i.e. not the mobile packages. The shared library (which is what would be impacted by this change) currently makes up the majority of the size of the JavaScript package (~ 90%). So, if we were to include two copies of this library in the JavaScript package we would come close to doubling the size of the package. Another option would be to create and publish an additional package for the ODBC support (e.g. aries-askar-odbc). |
I meant instead of including two copies of this library, to just always include ODBC in the main binary of Aries Askar for server packages (so Linux, macOS, Windows). But not in the Android / iOS builds. If you do want to do it dynamically, for JS it would be quite easy to modify this script based on env variables: https://github.com/hyperledger/aries-askar/blob/main/wrappers/javascript/packages/aries-askar-nodejs/scripts/install.js#L13, |
@TimoGlastra What you are suggesting was one of my original proposals (see one of my earlier comments). My concern however is that this change will introduce a dependency on a new shared library - libodbc.so. On a Windows machine this is installed by default, and so shouldn't be a problem. On other platforms this shared library must be installed manually. Is it acceptable to include a dependency on a new shared library which is not available in default platform installs? |
Can't speak for the other maintainers, but if the loading of the library is only done when you set type of the wallet to odbc then i think it's fine? (Given the bundle size increase is manageable). If i want to use Postgres and now first need to install this lib, that would be less ideal. Is it possible to only load the external lib if when you use odbc as a backend? |
The only way that I can think of to only load the external lib if ODBC is used as a backend would be to provide two versions of the shared library in the server package (one with, and one without ODBC support), and then the JavaScript module would selectively choose the correct library to use at runtime via some kind of setting (maybe an environment variable). Alliteratively, as already suggested, the install.js script can be modified to install the correct version of the library during the install operation based on something like an environment variable. I'm open to other suggestions as well. |
This thread has gone silent for a while and so I want to roughly explain how I want to add ODBC support so that I can get some level of approval before diving into the implementation. A new backend will be added called 'odbc' - this backend will be included as a new 'feature' and will be selected with an "odbc://..." database URL scheme. In order to add ODBC support we need to use the odbc_api Rust package - which in turn introduces a dependency on the ODBC shared library - which is not installed by default. Everyone agrees that it is a bad idea to introduce this dependency unless you actually request the ODBC support. To help overcome this issue I propose the following:
At the moment I only plan to concentrate on the NodeJS wrapper. At a future point someone could make corresponding changes to the python wrapper. Does anyone have any objections, or suggested changes? Thanks. |
@scottexton I'm okay with this approach. One alternative would be to add support for dynamically loaded backends, but I suspect that would be a lot of work, especially with the current design of the backend interface. |
@andrewwhitehead Thanks for the approval. I also thought about the idea of having a dynamically loaded backend, but figured that this was additional effort. The ODBC approach should allow any database which provides an ODBC driver to be supported. I am making some good progress on the code changes and hope to have a PR ready for review in the next week or so. |
A pull request (#319) has been created with the ODBC driver changes. The driver is protected by the 'odbc' feature and is not currently enabled. A subsequent pull request will introduce the alternate shared library (as previously discussed in this issue). |
Askar currently only supports SqlLite and Postgres, but we also need support additional databases, e.g. DB2 and MSSQL (which are not currently supported by sqlx). The easiest way to add support for additional databases is to add support for the Rust 'odbc-api' crate. Unfortunately this would also introduce a dependency on the ODBC shared library.
If I was to add ODBC support, would the code changes be accepted back into the project?
Also, do you believe that there would be a problem within introducing a runtime dependency on the ODBC shared library, or will we need to somehow dynamically load the ODBC shared library at runtime if the ODBC database type is selected?
Thanks.
The text was updated successfully, but these errors were encountered: