jcs / rubywarden

An unofficial, mostly Bitwarden-compatible API server written in Ruby (Sinatra and ActiveRecord)
ISC License
592 stars 49 forks source link

Integrate ActiveRecord #44

Closed universal closed 6 years ago

universal commented 6 years ago

see also #43 , just for simplification I copy the text I wrote there, the ticket can probably be closed:

I'm currently having a look at bringing in activerecord for the db stuff. It would open up the possibility to easily use other db systems. It would also probably simplify serializing data to and from the db in json format.

Some questions came up:

  1. The standalone-migrations gem in the current version depends on nearly everything of rails. The question is if that is something that is ok? It handles all the required rake tasks. The alternative is to just implement("copy from examples") the rake tasks.

sinatra-active-record gem provides the rake tasks and the integration with sinatra. If the last commit is not applied, the standalone-migrations gem is used. Both versions should just work fine.

  1. Migrating the current db: As it is, the current db can not be dumped into the ar-schema format since ar does not know how to handle the "STRING" column type, which doesn't even seem to be a valid sqlite column type and sqlite just seems to ignore it. So either just live with the current db and only implement changes to it using the ar-migrations or create a new one using migrations and definitions from rails and then migrate the data only.

There is a migration tool which should work fine with converting the database.

  1. Should the uuid string primary / foreign keys be used or use the default integer based keys from activerecord and just keep the uuid columns as unique ones?

uuid primary / foreign keys kept

  1. Should the manual serialization of some fields be kept or should it be adopted that the serialize :some_field helper from rails is used and a dedicated json serializer is added?

rails serialize helper with the JSON serializer used.

Current status:

Further refactorings

Open Questions

universal commented 6 years ago

using janko-m/sinatra-activerecord might be the more logical choice...

universal commented 6 years ago

Switching to sinatra-activerecord isn't complicated and reduces the number of gems required "from" rails. I've created a branch for that based off this one.

I've so far only run the specs, but haven't tested against the browser client. The specs are passing now. I'll manually check the browser plugin next.

If someone wants to give it a try, I highly suggest not doing it in a production env!

bundle --with migrate
# the following will migrate the development.sqlite3 file to the "ar-format"
# it will backup the original file to development.sqlite3.CURRENT_TIMESTAMP
ruby tools/migrate_to_ar.rb -e development
env RACK_ENV=development ALLOW_SIGNUPS=1 bundle exec rackup -p 4567 config.ru
universal commented 6 years ago

This should now be complete, some further testing would be very much appreciated! I checked it against my database and it seems to work fine and the firefox and chrome browser clients. I currently don't have a mobile device handy for testing.

Proper handling of Identies and Cards depends on #47.

universal commented 6 years ago

@jcs any feedback on this? If you don't want to integrate this, that is totally fine with me, but at least some comment would be nice :-) If you are busy with other stuff and can't look at it right now, I don't mind keeping it open for as long as it takes.

jcs commented 6 years ago

If you fix the merge conflicts I can try it out. I'm not opposed to switching to AR, there is just a ton of stuff in your PRs (this one and #52) so I don't know what is done and what you're still working on.

universal commented 6 years ago

This one is just switching to using active-record. The other one is all the additional stuff like attachments etc... I will fix the merge conflicts and ping you again when i'm done :-)

universal commented 6 years ago

@jcs I updated the commits. It should now merge just fine. I also added the sinatra-active-record commit to this pull-request, since I feel this is the more logical choice.

jcs commented 6 years ago

Seems to work here with some minor changes, I'll clean those up after merging.

Thanks for all the work involved in this.