-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add client JSON storage API endpoint #1405
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<?php | ||
|
||
class StorageTest extends PHPUnit\Framework\TestCase | ||
{ | ||
//------------------------------------------------------------------------ | ||
// Basic JSON storage test | ||
|
||
public function test_valid_json(): void | ||
{ | ||
$storage = new JsonStorage("username"); | ||
$storage->set("setting", "{}"); | ||
$value = $storage->get("setting"); | ||
$this->assertEquals("{}", $value); | ||
$value = $storage->delete("setting"); | ||
$this->assertEquals(null, $value); | ||
} | ||
|
||
public function test_invalid_json(): void | ||
{ | ||
$this->expectException(ValueError::class); | ||
$storage = new JsonStorage("username"); | ||
$storage->set("setting", "blearg"); | ||
} | ||
|
||
//------------------------------------------------------------------------ | ||
// API storage test | ||
|
||
public function test_valid_storagekey(): void | ||
{ | ||
global $api_storage_keys; | ||
$api_storage_keys = ["valid"]; | ||
|
||
$storage = new ApiStorage("valid", "username"); | ||
$storage->set("{}"); | ||
$value = $storage->get(); | ||
$this->assertEquals("{}", $value); | ||
$value = $storage->delete(); | ||
$this->assertEquals(null, $value); | ||
} | ||
|
||
public function test_invalid_storagekey(): void | ||
{ | ||
global $api_storage_keys; | ||
$api_storage_keys = []; | ||
|
||
$this->expectException(ValueError::class); | ||
|
||
$storage = new ApiStorage("invalid", "username"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
$relPath = '../../../pinc/'; | ||
include_once($relPath.'base.inc'); | ||
|
||
// ------------------------------------------------------------ | ||
|
||
echo "Creating json_storage table...\n"; | ||
|
||
$sql = " | ||
CREATE TABLE json_storage ( | ||
username varchar(25) NOT NULL, | ||
setting varchar(32) NOT NULL, | ||
value json NOT NULL, | ||
timestamp int NOT NULL default 0, | ||
PRIMARY KEY (username, setting) | ||
) | ||
"; | ||
|
||
mysqli_query(DPDatabase::get_connection(), $sql) or die(mysqli_error(DPDatabase::get_connection())); | ||
|
||
// ------------------------------------------------------------ | ||
|
||
echo "\nDone!\n"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,9 @@ class ApiRouter | |
private TrieNode $root; | ||
/** @var array<string, callable> */ | ||
private $_validators; | ||
/** @var mixed */ | ||
private $_response; | ||
private bool $_raw_response = false; | ||
|
||
public function __construct() | ||
{ | ||
|
@@ -74,7 +77,8 @@ class ApiRouter | |
if (!$handler) { | ||
throw new MethodNotAllowed(); | ||
} | ||
return $handler($method, $data, $query_params); | ||
$this->_response = $handler($method, $data, $query_params); | ||
return $this->_response; | ||
} | ||
|
||
public function add_validator(string $label, callable $function): void | ||
|
@@ -93,6 +97,35 @@ class ApiRouter | |
throw new InvalidAPI(); | ||
} | ||
|
||
/** @return mixed */ | ||
public function request(bool $raw = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function could stay in index.php There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this here to keep the encoding and decoding together. And because I had to move the encoding here this came along with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems reasonable. I've had some further thoughts about this and deleted my other ananswered comments which had overlooked a change you made in index.php. It's difficult to see the whole picture in this github view so I dowloaded the files to look at them more carefully. |
||
{ | ||
if ($raw) { | ||
return file_get_contents('php://input'); | ||
} else { | ||
$json_object = json_decode(file_get_contents('php://input'), true); | ||
if ($json_object === null) { | ||
throw new InvalidValue("Content was not valid JSON"); | ||
} | ||
return $json_object; | ||
} | ||
} | ||
|
||
public function response(bool $raw = false): string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We wouldn't need this if it was incorporated as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I wanted to keep the encoding and decoding together and they got moved in here, breaking them out into clear functions makes it clearer what's going on IMHO. |
||
{ | ||
if ($raw || $this->_raw_response) { | ||
return $this->_response; | ||
} else { | ||
return json_encode($this->_response, JSON_PRETTY_PRINT | | ||
JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES); | ||
} | ||
} | ||
|
||
public function set_raw_response(): void | ||
{ | ||
$this->_raw_response = true; | ||
} | ||
|
||
public static function get_router(): ApiRouter | ||
{ | ||
/** @var ?ApiRouter */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1338,6 +1338,58 @@ paths: | |
404: | ||
$ref: '#/components/responses/NotFound' | ||
|
||
/storage/{storagekey}: | ||
get: | ||
tags: | ||
- storage | ||
description: Get JSON blob stored for this storage key | ||
parameters: | ||
- name: storagekey | ||
in: path | ||
description: Storage key | ||
required: true | ||
schema: | ||
type: string | ||
responses: | ||
200: | ||
description: JSON blob | ||
content: | ||
application/json: | ||
schema: | ||
type: object | ||
put: | ||
tags: | ||
- storage | ||
description: Save JSON blob for this storage key | ||
parameters: | ||
- name: storagekey | ||
in: path | ||
description: Storage key | ||
required: true | ||
schema: | ||
type: string | ||
responses: | ||
200: | ||
description: JSON blob that was persisted | ||
content: | ||
application/json: | ||
schema: | ||
type: object | ||
Comment on lines
+1373
to
+1377
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The timestamp is arguably just as useless as the JSON blob because it's always Open to ideas / thoughts on what API clients should "expect" from a successful PUT / PATCH / etc beyond the HTTP status code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure as it represents the last modified timestamp from the server's perspective. However I do think there is a lot more value to returning this compared to the blob that was sent and is known to the client. The timestamp would allow the client to implement some caching (e.g. don't check with the server if the timestamp is fresh enough), lower the bandwidth needed (by return the JSON object on a
I don't think we need to return anything from the API on success: the return code says if it was successful and we can return an optional error field to return the cause of errors to the client. Those 2 are what a minimal client would expect. We can always add more as we understand our needs more. |
||
delete: | ||
tags: | ||
- storage | ||
description: Delete JSON blob for this storage key | ||
parameters: | ||
- name: storagekey | ||
in: path | ||
description: Storage key | ||
required: true | ||
schema: | ||
type: string | ||
responses: | ||
200: | ||
description: JSON blob was deleted | ||
|
||
components: | ||
securitySchemes: | ||
ApiKeyAuth: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this line now?