Add client JSON storage API endpoint #1405

cpeel · 2024-12-21T18:45:44Z

We want the new proofreading interface to be able to persist client configuration details across browsers and devices. This requires that some configuration values are stored server-side. To enable this, allow clients to store opaque JSON blobs on the server, up to one blob per client per user.

Please read the updates to SETUP/API.md on important caveats about this endpoint and ensure we're OK with this from a server-side perspective as well as the client side.

This PR introduces a new generalized JsonStorage class and database table with an ApiClientStorage layer on top of that wired into the API. When JSON blobs are updated in the JsonStorage class their timestamp field in the database is also updated. This field isn't currently made available via the code right now but seems like it would be useful if, for instance, we want to clean up client blobs older than some period of time. I'm open to thoughts on this.

curl examples to test this:

export API_KEY=review

# Save a json blob
curl -i -H "Accept: application/json" -H "X-API-KEY: $API_KEY" \
    -X PUT -d '{"key": 1}' "https://www.pgdp.org/~cpeel/c.branch/client-storage/api/v1/storage/clients/newpi"

# Fetch a json blob
curl -i -H "Accept: application/json" -H "X-API-KEY: $API_KEY" \
    -X GET "https://www.pgdp.org/~cpeel/c.branch/client-storage/api/v1/storage/clients/newpi"

# Delete a json blob
curl -i -H "Accept: application/json" -H "X-API-KEY: $API_KEY" \
    -X DELETE "https://www.pgdp.org/~cpeel/c.branch/client-storage/api/v1/storage/clients/newpi"

This is likely to have minor merge conflicts with #1398 and I'll resolve those after that gets merged.

70ray · 2024-12-22T09:58:02Z

SETUP/API.md

+
+Some important notes about this feature:
+* Client storage is one blob per user per client. Said another way: clients are
+  only able to store one blob per `clientid` and that blob is only for the user


Shouln't this be 'one blob per userid ?

It's both. It's one per clientid and by the user that is authenticated. I'll change it slightly to maybe make it clearer.

70ray · 2024-12-22T10:20:47Z

pinc/JsonStorage.inc

+        // just shy of 32MB. That seems plenty big.
+        $sql = sprintf(
+            "
+            INSERT INTO json_storage


Does this amount to the same thing as 'REPLACE'? Sorry if this is a silly question but I have very little experience of databases.

It does, yes. This is a MySQL-ism and not part of an SQL standard and we use it pretty frequently in the code.

70ray · 2024-12-22T10:36:57Z

api/v1_storage.inc

+
+    $storage = new ApiClientStorage($clientid, $pguser);
+    if ($method == "GET") {
+        return json_decode($storage->get());


Should not the client decode and enode the JSON data so we are just dealing with strings here?

The API automatically converts returns to JSON values so if we don't decode this into an object we stringify it twice.

70ray · 2024-12-22T10:40:41Z

pinc/ApiClientStorage.inc

+    {
+        global $api_client_storage_keys;
+
+        if (!in_array($client, $api_client_storage_keys)) {


Would the client not already have been validated before we get here?

It would. I did it here too to ensure it's enforced at the data layer not just the user input layer if this is used elsewhere for some reason.

70ray

This looks like a good overall structure, just a few comments about details.

jchaffraix · 2024-12-22T16:38:52Z

api/dp-openapi.yaml

+          description: JSON blob that was persisted
+          content:
+            application/json:
+              schema:
+                type: object


Wouldn't it make sense to return the timestamp (something the client could use for caching) rather than the JSON blob that was sent by the client?

The timestamp is arguably just as useless as the JSON blob because it's always now(). I was uncertain what a good return value would be so I modeled what we do for PUTs for word lists -- and what the product at my job does for PUTs and PATCHes -- which is to return the object that was actually serialized.

Open to ideas / thoughts on what API clients should "expect" from a successful PUT / PATCH / etc beyond the HTTP status code.

api/v1_storage.inc

pinc/ApiClientStorage.inc

jchaffraix · 2024-12-22T16:49:00Z

pinc/JsonStorage.inc

+     */
+    public function get(string $setting): ?string
+    {
+        $value = null;


We should move defining $value where need it before the loop on l.69.

api/v1_validators.inc

jchaffraix · 2024-12-22T17:01:19Z

SETUP/API.md

+To enable this feature, add a string for the client to the
+`_API_CLIENT_STORAGE_KEYS` configuration setting and have the client use that
+string with the endpoint as the `clientid`.


I am having a hard time understanding the purpose of the API client storage keys. If the end goal is to allow any client to store this configuration down the road (which means that a user could trivially get one of our keys), is this some temporary measure to prevent arbitrary storage from storing JSON in our DB?

You have the gist of it, but it's not temporary.

Keep in mind that every user has de facto API access via PHP sessions -- that's how the Page Browser users the API and how the new proofreading UI will too. If we don't have some restriction, any logged in user could fill up our server with random JSON blobs. The clientid means that each user can have only at most len(API_CLIENT_STORAGE_KEYS) JSON blobs and those have a maximum size.

Perhaps "client" is the wrong word for this? I am using it in the "JS client that uses the APIs". Would "app" or some other word be clearer?

cpeel

Thanks for the questions and updates. I'll get the (excellent) code suggestions done later today.

cpeel · 2024-12-22T17:40:14Z

api/v1_storage.inc

+
+    $storage = new ApiClientStorage($clientid, $pguser);
+    if ($method == "GET") {
+        return json_decode($storage->get());


The API automatically converts returns to JSON values so if we don't decode this into an object we stringify it twice.

cpeel · 2024-12-22T17:40:54Z

pinc/JsonStorage.inc

+        // just shy of 32MB. That seems plenty big.
+        $sql = sprintf(
+            "
+            INSERT INTO json_storage


It does, yes. This is a MySQL-ism and not part of an SQL standard and we use it pretty frequently in the code.

cpeel · 2024-12-22T17:46:23Z

SETUP/API.md

+To enable this feature, add a string for the client to the
+`_API_CLIENT_STORAGE_KEYS` configuration setting and have the client use that
+string with the endpoint as the `clientid`.


You have the gist of it, but it's not temporary.

Keep in mind that every user has de facto API access via PHP sessions -- that's how the Page Browser users the API and how the new proofreading UI will too. If we don't have some restriction, any logged in user could fill up our server with random JSON blobs. The clientid means that each user can have only at most len(API_CLIENT_STORAGE_KEYS) JSON blobs and those have a maximum size.

cpeel · 2024-12-22T17:48:54Z

api/dp-openapi.yaml

+          description: JSON blob that was persisted
+          content:
+            application/json:
+              schema:
+                type: object


The timestamp is arguably just as useless as the JSON blob because it's always now(). I was uncertain what a good return value would be so I modeled what we do for PUTs for word lists -- and what the product at my job does for PUTs and PATCHes -- which is to return the object that was actually serialized.

Open to ideas / thoughts on what API clients should "expect" from a successful PUT / PATCH / etc beyond the HTTP status code.

cpeel · 2024-12-22T17:50:10Z

pinc/ApiClientStorage.inc

+    {
+        global $api_client_storage_keys;
+
+        if (!in_array($client, $api_client_storage_keys)) {


It would. I did it here too to ensure it's enforced at the data layer not just the user input layer if this is used elsewhere for some reason.

pinc/ApiClientStorage.inc

SETUP/tests/ci_configuration.sh

Add a generic JSON-based storage class and an extension of it that will be used in the API to store client blobs. This is a similar structure to the Settings class / user_settings table but enforces a JSON value.

cpeel · 2024-12-23T00:04:37Z

PR feedback so far incorporated and resolved the merge conflicts with the documents API PR.

cpeel requested review from jchaffraix, chrismiceli, 70ray and srjfoo December 21, 2024 18:45

cpeel self-assigned this Dec 21, 2024

70ray reviewed Dec 22, 2024

View reviewed changes

jchaffraix reviewed Dec 22, 2024

View reviewed changes

cpeel commented Dec 22, 2024

View reviewed changes

SETUP/tests/ci_configuration.sh Outdated Show resolved Hide resolved

cpeel added 2 commits December 22, 2024 23:53

Add JSON storage class

33f354c

Add a generic JSON-based storage class and an extension of it that will be used in the API to store client blobs. This is a similar structure to the Settings class / user_settings table but enforces a JSON value.

Wire JSON storage into the API

974c9b6

cpeel force-pushed the client-storage branch from ac44218 to 974c9b6 Compare December 23, 2024 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client JSON storage API endpoint #1405

Add client JSON storage API endpoint #1405

cpeel commented Dec 21, 2024

70ray Dec 22, 2024

cpeel Dec 22, 2024

70ray Dec 22, 2024

cpeel Dec 22, 2024

70ray Dec 22, 2024

cpeel Dec 22, 2024

70ray Dec 22, 2024

cpeel Dec 22, 2024

70ray left a comment

jchaffraix Dec 22, 2024

cpeel Dec 22, 2024

jchaffraix Dec 22, 2024

jchaffraix Dec 22, 2024

cpeel Dec 22, 2024

cpeel Dec 22, 2024

cpeel left a comment

cpeel Dec 22, 2024

cpeel Dec 22, 2024

cpeel Dec 22, 2024

cpeel Dec 22, 2024

cpeel Dec 22, 2024

cpeel commented Dec 23, 2024

Add client JSON storage API endpoint #1405

Are you sure you want to change the base?

Add client JSON storage API endpoint #1405

Conversation

cpeel commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

70ray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpeel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpeel commented Dec 23, 2024