Add key value store

[xylix]

This PR adds key/value storage functionality. Currently implementing changes based on review feedback.

• implement PR feedback
  • rename create_value() to insert_value or something of the sort
  • 204 -> 404 for missing value
  • log errors
  • Add key length limit (maybe 100-128?)
  • rename fns frddom x_value to x_key_value
  • use ? operator in settings.rs
  • nosuchvalue Error -> no such key error
  • Maybe rename functions from insert to set?
  • add response body to _set_setting_request parameters in tests

new features coming into this PR:

• Take json object as input request body {"key": "sample-key", "value": "sample_Value"}
• Return json objects (probably `{"key": "settings.sample-key", "value": "sample_Value"}
• Add last_modified column to sql
  • Test timestamps in some sane way
• endpoint for listing all settings key-values
  • test

Open questions:

• Maybe different status msgs (created vs updated) No.
• Maybe return timestamp on setting set No.

[johan-bjareholt]

Implementation wise looks very good, mostly have questions regarding API.

I think a last_modified should be added, possibly also "created".

I don't think we should just call it "key_value". The name should clearly define what it's made for. Maybe just call it "settings" with "sections"?

Another question would be if we should allow any plaintext like the current implementation or force it to contain valid JSON? Enforcing it to JSON could make APIs more clear. In that case we could also for example make the API return something line { "settings": ..., "last_modified": "2020-02-20..." }

Nice to see proper tests already.

Really happy to see these changes, thanks a lot! :-)

[xylix]

I think a last_modified should be added, possibly also "created".

What would be the use cases for a "created" field if there already exists a last_modified one?

I'll work on adding the last_modified column.

I don't think we should just call it "key_value". The name should clearly define what it's made for. Maybe just call it "settings" with "sections"?

I personally would internally call it key_value for reusability, but perhaps rename the API endpoints to settings or something of the sort. Can you clarify did you wish to change everything or just some part of the chain?

[johan-bjareholt]

What would be the use cases for a "created" field if there already exists a last_modified one?

I don't know, that's why i said "possibly". It's fine to skip that.

I personally would internally call it key_value for reusability, but perhaps rename the API endpoints to settings or something of the sort. Can you clarify did you wish to change everything or just some part of the chain?

Could be done, but if you want it to be reusable you should have a separate namespace for settings, maybe just prefix each key like "settings.key".

The risk of having it too general-purpose is that it might be that if we start using it for too much it might be able to do a lot but being bad at everything. For example if we want to start saving icons for every application we could base64 all of them and put them inside. The proper solution would be to add a new table to the database which saves it as blobs and add a separate API for it, but since it's so general purpose nothings stopping it from being used like that even though its a suboptimal solution. Restricting APIs down to what they're intended to do is good practice if it doesn't disturb usability. If an API can be misused it's just a matter of time before it will be misused.

So my opinion is that it's important to change the API name to something more specific as the API should preferably never change once settled. What it's called inside the code doesn't matter as much, but I think we should avoid using it for more than once purpose.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@9324508). Click here to learn what that means.
The diff coverage is 78.98%.

@@            Coverage Diff            @@
##             master      #87   +/-   ##
=========================================
  Coverage          ?   80.34%           
=========================================
  Files             ?       40           
  Lines             ?     3689           
  Branches          ?        0           
=========================================
  Hits              ?     2964           
  Misses            ?      725           
  Partials          ?        0

Impacted Files	Coverage Δ
aw-datastore/src/lib.rs	0% <ø> (ø)
aw-server/src/endpoints/mod.rs	33.8% <100%> (ø)
aw-models/src/key_value.rs	50% <50%> (ø)
aw-server/src/endpoints/settings.rs	74.57% <74.57%> (ø)
aw-datastore/src/datastore.rs	80.89% <75%> (ø)
aw-datastore/src/worker.rs	72.6% <77.27%> (ø)
aw-server/tests/api.rs	96.25% <91.66%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9324508...28f4c75. Read the comment docs.

[ErikBjare]

Awesome PR. Just a minor naming thing otherwise I'm happy with it.

[ErikBjare]

Also, about the namespace stuff. I always imagined that settings simply have a settings prefix/namespace (delimited with a .) but that the API endpoint isn't restricted to that namespace or usecase. However, I can see the issue @johan-bjareholt raised about the potential for API misuse, so maybe that's for the best anyway.

Apart from that, I realize now that there is no way to list all settings keys. Maybe the /api/0/settings endpoint should simply return all keys (and their values?) under the settings namespace?

And I definitely think we should add the last_modified column. A created column is not needed.

[xylix]

Hm yeah the insert was used to try be consistent with the internal bucket and events methods. I do agree with set being a bit clearer though, especially in the API endpoints code.

I'm currently working on adding a /settings endpoint returning a json of all the {key, value} pairs. I'd guess having the namespacing in server-side would be most clear along with implementing a listing feature as an API endpoint.

Agreed with the last_modified point as well.

[xylix]

@johan-bjareholt My implementation behind the "get all settings" endpoint is failing with an unwrap issue Im having problems debugging. (backtrace: https://gist.github.com/xylix/0b09370eb57dc42df8ff5024f20c16b1 , commit 488762c probably easiest way to see the code)

Somehow the receiver is failing to unwrap but no other addition I have done to the worker.rs has had this problem, even when the SQL queries were failing. Now I think (I tried validating in multiple ways) the SQL and unwrapping the response is correct, but the worker is getting some weird result.

Have you had any problems like this / any tips on debugging this one?

[johan-bjareholt]

Apart from that, I realize now that there is no way to list all settings keys. Maybe the /api/0/settings endpoint should simply return all keys (and their values?) under the settings namespace?

I'm currently working on adding a /settings endpoint returning a json of all the {key, value} pairs.

I think you should only return the keys, I hope we never get a use-case for dumping all the values. It's an open API, we have no idea what's going to be put in there so dumping all values could become very slow depending on what people put there.

[johan-bjareholt]

My implementation behind the "get all settings" endpoint is failing with an unwrap issue Im having problems debugging. (backtrace: https://gist.github.com/xylix/0b09370eb57dc42df8ff5024f20c16b1 , commit 488762c probably easiest way to see the code)

Replace the "expect" with an unwrap. Otherwise you can only see in the backtrace that it's an RecvError but can't see the reason for the RecvError. My first guess would be that the worker fails ungracefully with something and drops the sender and if there's no sender the receiver part will get an RecvError.

[xylix]

I guess it makes sense to change the list endpoint to keys only. I'll do that.

Also I changed the .expect to .unwrap, It didn't really improve the stacktrace :/ (https://gist.github.com/xylix/1b85a4fe805241862440ea8cd420ef44 )

[johan-bjareholt]

https://github.com/ActivityWatch/mpsc_requests/blob/master/src/lib.rs#L128

The only time RecvError is returned is if the response channel is broken so something is going wrong in the worker thread. If you run with a debugger like gdb on the test binary you will see what the other thread is doing (or of it is still alive).

[xylix]

Allright I did some debugging and peeked at the rusqlite code, it seems like the problem was the sqlite LIKE '?%' in my query, sqlite didn't like the escaping by the ?% combination. For some reason the sql didn't err, but panicked and crashed the worker thread.

Will probably have a working commit in a bit.

[xylix]

Last modified column has now been added, altough the tests are still lacking. I couldn't figure out a sane way to test the timestamps of settings since there's really no way for the tests now at what time an insertion query finished.

[ErikBjare]

For the test: Just check that last_modified is in the range (timestamp just before set, timestamp just after set). Should be enough.

[xylix]

There's still the open question about different return statuses on first insert vs update, and also I was thinking that maybe it'd be useful to return timestamp on setting set, but I don't really have any idea about it's uses outside of confirming time of insertion in client side, which shouldn't be needed anyway.

Otherwise I think we are merge ready. I'll re-request the review to clean up the red mark.

[johan-bjareholt]

A few minor comments, looks really good now!

[johan-bjareholt]

Looks good to me now, should we merge @ErikBjare ?

[ErikBjare]

Looks really good! Nice work!

[ErikBjare]

I'm happy! Merging, and then fixing #88.