Skip to content
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

use proper types in API #948

Merged
merged 1 commit into from
Apr 23, 2018
Merged

use proper types in API #948

merged 1 commit into from
Apr 23, 2018

Conversation

niol
Copy link
Collaborator

@niol niol commented Jun 16, 2017

No description provided.

@@ -209,7 +209,7 @@ public function write() {

$return = [
'success' => true,
'id' => $id,
'id' => intval($id),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be (int) $id cast for consistency (will be enforced once refactoring PR is merged).

@@ -22,7 +22,7 @@ public static function insert($query, array $params) {
\F3::get('db')->exec($query, $params);
$res = \F3::get('db')->exec('SELECT LAST_INSERT_ID() as lastid');

return $res[0]['lastid'];
return intval($res[0]['lastid']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -22,7 +22,7 @@ public static function insert($query, array $params) {
\F3::get('db')->exec($query, $params);
$res = \F3::get('db')->exec('SELECT last_insert_rowid() as lastid');

return $res[0]['lastid'];
return intval($res[0]['lastid']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@niol
Copy link
Collaborator Author

niol commented Aug 2, 2017

Are there still changes requested on this?

@jtojnar
Copy link
Member

jtojnar commented Aug 2, 2017

Sorry, this is stalling on my end. I will finish the API schema today and then we can move forward.

@jtojnar
Copy link
Member

jtojnar commented Nov 2, 2017

Unfortunately the OpenAPI 2 is too limited so I was not able to finish it. This will have to wait until #993 is done.

@niol niol force-pushed the ppr/apitypes branch 2 times, most recently from 7d198ff to 0a80317 Compare January 11, 2018 10:10
@jtojnar
Copy link
Member

jtojnar commented Jan 11, 2018

Okay, I finally finished the OpenAPI schema, could you please take a look if you see anything too bad? Unfortunately the tooling still did not catch up with OpenAPI 3 so we cannot test it automatically yet but it should still be more accurate than the wiki.

@jtojnar
Copy link
Member

jtojnar commented Mar 9, 2018

I am getting an error with this branch rebased onto master

$ curl 'http://localhost/selfoss/?ajax=true'
error occured: explode() expects parameter 2 to be string, array given
[selfoss/vendor/bcosca/fatfree-core/base.php:2190] Base->error()
[selfoss/daos/Items.php:83] explode()
[selfoss/controllers/Index.php:218] daos\Items->get()
[selfoss/controllers/Index.php:52] controllers\Index->loadItems()
[selfoss/vendor/bcosca/fatfree-core/base.php:1806] controllers\Index->home()
[selfoss/vendor/bcosca/fatfree-core/base.php:1627] Base->call()
[selfoss/index.php:80] Base->run()

@jtojnar jtojnar added this to the 2.19 milestone Mar 9, 2018
@niol
Copy link
Collaborator Author

niol commented Mar 9, 2018

Okay this is only showing when public=1.

@jtojnar
Copy link
Member

jtojnar commented Mar 9, 2018

/sources/list still contains unsplit tags.

$ curl -X POST "http://localhost/selfoss/login?username=test&password=pass" -b jar -c jar
{"success":true}⏎
$ curl 'http://localhost/selfoss/sources/list' -b jar -c jar | jq 'map(.tags)'

@jtojnar
Copy link
Member

jtojnar commented Mar 9, 2018

POST /source does not seem to accept arrays:

$ curl -X POST "http://localhost/selfoss/source" -H  "Content-Type: application/json" -d '{"title": "Test", "spout": "spouts\\rss\\feed", "tags": ["IOT", "test"], "url": "https://github.com/SSilence/selfoss/commits/master.atom"}' -b jar -c jar
nastala chyba: htmlspecialchars() expects parameter 1 to be string, array given
[selfoss/vendor/bcosca/fatfree-core/base.php:2190] Base->error()
[selfoss/controllers/Sources.php:156] htmlspecialchars()
[selfoss/vendor/bcosca/fatfree-core/base.php:1806] controllers\Sources->write()
[selfoss/vendor/bcosca/fatfree-core/base.php:1627] Base->call()
[selfoss/index.php:80] Base->run()

@jtojnar
Copy link
Member

jtojnar commented Mar 9, 2018

The effect on the public API seems to be minor: api.patch

@jtojnar
Copy link
Member

jtojnar commented Apr 21, 2018

The client side will need to be updated as well:

screen shot 2018-04-21 at 14 56 01

@jtojnar
Copy link
Member

jtojnar commented Apr 23, 2018

Thanks, @niol. I will test it a little more but it should be good to go. Initially, I wanted to have tests ready first but as they say, perfect is the enemy of good – we can just fix anything broken later.

@jtojnar
Copy link
Member

jtojnar commented Apr 23, 2018

Another issue when saving a source, tags should also be sent as an array:

an error occured: implode(): Invalid arguments passed
[vendor/bcosca/fatfree-core/base.php:2190] Base->error()
[controllers/Sources.php:156] implode()
[vendor/bcosca/fatfree-core/base.php:1806] controllers\Sources->write()
[vendor/bcosca/fatfree-core/base.php:1627] Base->call()
[index.php:80] Base->run()

@jtojnar
Copy link
Member

jtojnar commented Apr 23, 2018

Another one in OPML export:

an error occured: explode() expects parameter 2 to be string, array given
[vendor/bcosca/fatfree-core/base.php:2190] Base->error()
[controllers/Opml.php:304] explode()
[vendor/bcosca/fatfree-core/base.php:1806] controllers\Opml->export()
[vendor/bcosca/fatfree-core/base.php:1627] Base->call()
[index.php:80] Base->run()

I was also thinking about changing the datatype in pgsql using something like

--- a/daos/pgsql/Database.php
+++ b/daos/pgsql/Database.php
@@ -227,6 +227,12 @@
                         'INSERT INTO version (version) VALUES (12)'
                     ]);
                 }
+                if (strnatcmp($version, '13') < 0) {
+                    \F3::get('db')->exec([
+                        "ALTER TABLE sources ALTER COLUMN tags SET DATA TYPE TEXT[] USING regexp_split_to_array(tags, E', ?');",
+                        'INSERT INTO version (version) VALUES (13)'
+                    ]);
+                }
             }
 
             // just initialize once
--- a/daos/pgsql/Statements.php
+++ b/daos/pgsql/Statements.php
@@ -84,7 +84,7 @@
      * @return string full statement
      */
     public static function csvRowMatches($column, $value) {
-        return "$value=ANY(string_to_array($column, ','))";
+        return "$value=ANY($column)";
     }
 
     /**
@@ -98,23 +98,7 @@
      *         expected types
      */
     public function ensureRowTypes(array $rows, array $expectedRowTypes) {
-        foreach ($rows as $rowIndex => $row) {
-            foreach ($expectedRowTypes as $columnIndex => $type) {
-                if (array_key_exists($columnIndex, $row)) {
-                    switch ($type) {
-                        // pgsql returns correct PHP types for INT and BOOL
-                        case \daos\PARAM_CSV:
-                            $value = explode(',', $row[$columnIndex]);
-                            break;
-                        default:
-                            $value = null;
-                    }
-                    if ($value !== null) {
-                        $rows[$rowIndex][$columnIndex] = $value;
-                    }
-                }
-            }
-        }
+        // pgsql uses correct PHP types
 
         return $rows;
     }

but the inconsistency between databases probably is not worth it.

We could probably clean the internal piping a bit, though:

--- a/controllers/Sources.php
+++ b/controllers/Sources.php
@@ -153,7 +153,7 @@
 
         // clean up title and tag data to prevent XSS
         $title = htmlspecialchars($data['title']);
-        $tags = htmlspecialchars(implode(',', $data['tags']));
+        $tags = array_map('htmlspecialchars', $data['tags']);
         $spout = $data['spout'];
         $filter = $data['filter'];
         $isAjax = isset($data['ajax']);
@@ -199,7 +199,6 @@
 
         // autocolor tags
         $tagsDao = new \daos\Tags();
-        $tags = explode(',', $tags);
         foreach ($tags as $tag) {
             $tagsDao->autocolorTag(trim($tag));
         }
--- a/daos/mysql/Sources.php
+++ b/daos/mysql/Sources.php
@@ -14,7 +14,7 @@
      * add new source
      *
      * @param string $title
-     * @param string $tags
+     * @param string[] $tags
      * @param string $spout the source type
      * @param mixed $params depends from spout
      *
@@ -38,7 +38,7 @@
      *
      * @param int $id the source id
      * @param string $title new title
-     * @param string $tags new tags
+     * @param string[] $tags new tags
      * @param string $spout new spout
      * @param mixed $params the new params
      *
--- a/controllers/Opml.php
+++ b/controllers/Opml.php
@@ -206,16 +206,16 @@
         $hash = md5($title . $spout . json_encode($data));
         if (array_key_exists($hash, $this->imported)) {
             $this->imported[$hash]['tags'] = array_unique(array_merge($this->imported[$hash]['tags'], $tags));
-            $tags = implode(',', $this->imported[$hash]['tags']);
+            $tags = $this->imported[$hash]['tags'];
             $this->sourcesDao->edit($this->imported[$hash]['id'], $title, $tags, '', $spout, $data);
             \F3::get('logger')->debug("OPML import: updated tags for '$title'");
         } elseif ($id = $this->sourcesDao->checkIfExists($title, $spout, $data)) {
             $tags = array_unique(array_merge($this->sourcesDao->getTags($id), $tags));
-            $this->sourcesDao->edit($id, $title, implode(',', $tags), '', $spout, $data);
+            $this->sourcesDao->edit($id, $title, $tags, '', $spout, $data);
             $this->imported[$hash] = ['id' => $id, 'tags' => $tags];
             \F3::get('logger')->debug("OPML import: updated tags for '$title'");
         } else {
-            $id = $this->sourcesDao->add($title, implode(',', $tags), '', $spout, $data);
+            $id = $this->sourcesDao->add($title, $tags, '', $spout, $data);
             $this->imported[$hash] = ['id' => $id, 'tags' => $tags];
             \F3::get('logger')->debug("OPML import: successfully imported '$title'");
         }
@@ -301,7 +301,7 @@
         $sources = ['tagged' => [], 'untagged' => []];
         foreach ($this->sourcesDao->get() as $source) {
             if ($source['tags']) {
-                foreach (explode(',', $source['tags']) as $tag) {
+                foreach ($source['tags'] as $tag) {
                     $sources['tagged'][$tag][] = $source;
                 }
             } else {

@niol
Copy link
Collaborator Author

niol commented Apr 23, 2018

Yes we could use pgsql datatypes or use an association table, I tried this some time ago but could not come up with better performance, so I did not go further.

@jtojnar jtojnar merged commit 6cd9feb into fossar:master Apr 23, 2018
jtojnar added a commit that referenced this pull request Jul 9, 2018
Feed was forgotten during the API clean up (#948)
resuting in the following error when opening `/feed`:

```
error occurred:
explode() expects parameter 2 to be string, array given
[selfoss/vendor/bcosca/fatfree-core/base.php:2190] Base->error()
[selfoss/controllers/Rss.php:76] explode()
[selfoss/vendor/bcosca/fatfree-core/base.php:1806] controllers\Rss->rss()
[selfoss/vendor/bcosca/fatfree-core/base.php:1627] Base->call()
[selfoss/index.php:80] Base->run()
```
jtojnar added a commit that referenced this pull request Oct 21, 2018
It was forgotten during the API clean-ups:

#948
@jtojnar jtojnar mentioned this pull request May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants