search

openwallet-foundation / askar

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

Add support for an ODBC backend #319

Closed scottexton closed 1 day ago

scottexton commented 1 month ago

This pull request adds support for an ODBC backend. At the moment the support is protected by the 'odbc' feature, and is not enabled by default. A subsequent pull request will enable this feature in an alternate shared library (so that we don't unnecessarily introduce a dependency on the UnixODBC shared library).

This pull request also contains a fix for an issue which was recently introduced in the JavaScript API. The 'descending' parameter is no longer required in the scan and fetchAll JavaScript API's.

Although I am an experienced C/C++ developer, this is my first attempt at developing in Rust - so I hope that I have not done anything too silly.

As a part of the testing of this change the store.test.ts script was successfully executed against a DB2 server.

andrewwhitehead commented 2 days ago

Sorry about the delay on this but we've been migrating things from Hyperledger to OpenWallet Foundation, and as part of that migration the JS wrapper is being split into a separate repo (https://github.com/openwallet-foundation/askar-wrapper-js-rn).

There are conflicts at the moment (probably mostly in Cargo.lock due to dependency updates) which I'm not able to resolve. I think the JS updates can be applied here, although the wrapper will be removed from here the changes can then be cherry-picked to the other repo.

One minor issue I see, you're using the lazy_static crate while once_cell is already in the dependency tree and can accomplish the same thing.

scottexton commented 2 days ago

@andrewwhitehead Thanks for the review. It looks like the performance is pretty terrible with the ODBC driver approach and so in the end we went a different way (sorry that I didn't let you know about this prior to you spending time on the review). I'm not sure what you want to do with this pull request? It won't be built by default and so I am happy for it to be merged, in case someone wants this capability in the future, or I am happy for the pull request to be closed.

andrewwhitehead commented 1 day ago

Ah okay, given that nobody is currently using it (or able to maintain it) I'll have to close the PR for now. It's there if somebody wants to revive the support. Thanks!