openwallet-foundation / askar

Secure storage designed for Hyperledger Aries agents.
Apache License 2.0
63 stars 51 forks source link

ODBC Support #314

Open scottexton opened 1 month ago

scottexton commented 1 month ago

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.

TimoGlastra commented 1 month ago

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

scottexton commented 1 month ago

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.

berendsliedrecht commented 1 month ago

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.

andrewwhitehead commented 1 month ago

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.

scottexton commented 1 month ago

@andrewwhitehead

Thanks for the response. I've included some following up questions/comments below:

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 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:

  1. Publish two versions of aries-askar to NPM - one with, and one without the ODBC feature.
  2. Dynamically load the ODBC library, using something like libloading. I'm not convinced that this would work, but it would remove the runtime dependency on liboidbc, unless specifically enabled.
  3. Add the code as a non-default feature, and we would need to compile the OpenSource code internally for our product.

What do you think?

This would probably be implemented as a single database backend and selected with an "odbc://..." database URL scheme. This makes sense.

swcurran commented 1 month ago

@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.

scottexton commented 1 month ago

@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.

scottexton commented 1 month ago

@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?

TimoGlastra commented 1 month ago

I think would be interesting to find out what the bundle size will be with, and without the odbc feature?

Compressed sizes are now as follows. If 3.7 becomes 3.8 i don't think it's worth it to publish two different versions for Node.JS (but the feature is still useful as we don't need it in the ios-android builds)

image
scottexton commented 1 month ago

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).

TimoGlastra commented 1 month ago

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,

scottexton commented 1 month ago

@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?

TimoGlastra commented 1 month ago

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?

scottexton commented 1 month ago

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.

scottexton commented 1 month ago

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:

  1. Two versions of the rust shared library will be produced, one without the odbc feature enabled, and one with the odbc feature enabled.
  2. When you do an npm install of the package both versions of the library will be downloaded and installed.
  3. A new function will be added to AriesAskar.ts to allow the version of the library to be selected (e.g. AriesAskar.setLibraryVersion(Library.ODBC)). This will change the library which is loaded within the getNativeAriesAskar() function. The non-ODBC version of the library will be used by default.

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.

andrewwhitehead commented 1 month ago

@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.

scottexton commented 1 month ago

@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.

scottexton commented 4 weeks ago

A pull request (https://github.com/hyperledger/aries-askar/pull/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).