Fix serialization of arrays of string in update #428

onaluf · 2020-03-20T09:08:22Z

This pull request fixes a bug I encountered with the PostgeSQL connector when updating the value of a model's property of type array of strings.

When creating the model no issue occurs but when updating it the value stored in the column turns into an invalid array, for example '{ "A","B","C"}' instead of '["A","B","C"]. After a bit of investigating I thought it was linked to the juggler giving the value in the query as an Array instead of a List object but even after fixing this the issue was still there. So I figured that it must be in the connector and changed it there. (This may be related to #342)

If you think this change belong somewhere else just tell me and I'll move it.

The added unit test just check that the object is still deserialisable after an update. It fails without the change and pass with it.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈 (done)

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Commit messages are following our guidelines

slnode · 2020-03-20T09:08:25Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

achrinza · 2020-04-12T04:54:33Z

@slnode test please

onaluf · 2020-04-12T13:49:22Z

Ok so I've seen the test that failed was that in the mean time my branch got behind master... I'll try to rebase it as soon as possible.

jannyHou

@onaluf Thank you for creating the PR! I left a few comments for refactoring the tests.
I need a bit more time to setup a postgresql server for testing, will review the code change asap

jannyHou · 2020-04-27T15:31:50Z

test/postgresql.test.js

+        },
+        function(callback) {
+          Model.findOne({where: {id: 1}}, function(err, r) {
+            r.should.have.property('productCodes');


please also add a check for productCodes's value:

should.equal(r.productCodes, ['AA', 'AC']);

jannyHou · 2020-04-27T15:53:39Z

test/postgresql.test.js

+          'id': {
+            'type': 'number', 'id': true, 'generated': true,
+          },
+          'productCodes': {


Can you add productCodes as a property of the existing model Post?
See its definition in https://github.com/strongloop/loopback-connector-postgresql/blob/dc47d13d5e15d5c31d75fb7c044d2e97542072df/test/postgresql.test.js#L61-L66

And you can add the tests after this one(it also tests patch):
https://github.com/strongloop/loopback-connector-postgresql/blob/dc47d13d5e15d5c31d75fb7c044d2e97542072df/test/postgresql.test.js#L193

jannyHou · 2020-04-27T15:57:15Z

test/postgresql.test.js

+          Model.destroyAll(callback);
+        },
+        function(callback) {
+          Model.create({productCodes: ['AA', 'AB']}, callback);


We support the promise chain of CRUD operations, example see
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/crud-with-options.test.js#L149

So you can write the chain calls as

Post.create(params) .then(()=> { return Post.updateAll(params) }) .then(()=> { // other function calls })

spirAde · 2020-04-27T19:02:12Z

Hello, guys. This solution doesn't work, because it's not a real postgresql array type in database. It will be convert just as text field type. You can easily check this, if you add postgresql settings in productCodes field

'productCodes': {
  'type': ['string'],
  'postgresql': {
    'dataType': 'varchar[]',
  }
},

After that the code of author will fail with error from pg: malformed array literal: ""["AA","AB"]"", because postgresql array type expects {'AA', 'AB'}.

@jannyHou FYI ^^

I'm going to create small app to reproduce this issue. I'll write here about it

onaluf · 2020-04-27T22:34:44Z

@jannyHou I've changed the test according to your pointers, hope it suits you.

@spirAde yes the array is saved as a string in the DB but it's then deserialized correctly. I think this is the correct behavior, at least that's the current way the connector behaves when creating the entry in the first place.... only the updating is problematic as far as I can tell.

onaluf · 2020-04-27T22:40:30Z

@spirAde Sorry I think I now get what you mean... if the dataType is explicitly set to varchar[] then it fails... however by default the array is saved as a string in the db... I'll try to see how to accommodate both cases.

onaluf · 2020-04-28T07:29:29Z

Ok @spirAde, @jannyHou I think this time I've got it. I've written the test for both cases and generalized the solution for non-string array.

The issue, as far as I understand it, was that toColumnValue received different type of values for arrays depending on the situation... on top of that the expected value varied based on the dataType of the field:

Creating an object always ended with val being a string (of a standard stringified array)
Updating an object always ended with val being an Array
Default dataType wanted a string
Array dataType wanted an Array

This is why I could create but not update my model AND people in #342 could update but not create theirs.

I hope the proposed solution is suitable but don't hesitate and I'll continue to improve it if needed.

onaluf · 2020-04-28T07:31:02Z

lib/postgresql.js

+    const isArrayDataType = prop.postgresql &&
+                            prop.postgresql.dataType &&
+                            (prop.postgresql.dataType.indexOf('[]') > -1);


There may be a more elegant way to detect this but I couldn't find one...

@onaluf is varchar[] the only possible data type? or datatype in format *[] are all valid conditions?

If varchar[] is the only valid one, you can use

const isArrayDataType = prop.postgresql && prop.postgresql.dataType === 'varchar[]';

instead.

spirAde · 2020-04-28T08:37:20Z

@onaluf thank you, it works now. It's awesome. I'm not sure about serialization of array and convert it to database text column type. From my perspective, loopback should supports the strict postgresql array type only, it means you always should pass postgresql dataType varchar[]. If user wants to store serialized array in text field in database, this should remain on the user side. In this case you can simplify your if-else conditions

if (prop.type instanceof Array && !(val instanceof Array)) {
  return JSON.parse(val);
}

But only maintainers can answer on this question. Anyway, @onaluf thank you again, best wishes and have a good day

jannyHou · 2020-05-02T05:32:27Z

I'm not sure about serialization of array and convert it to database text column type.

According to function buildColumnType, this is the expected behavior.

So the array type data is serialized to a string then deserialized to the array.

I think the reason is SQL databases have weaker support for object, and the DSL of LB is inspired by mongodb...So type like array and object are serialized.

jannyHou · 2020-05-02T04:56:49Z

test/postgresql.test.js

+        post.tags[1].should.eql('AB');
+        return Post.updateAll({where: {id: postId}}, {tags: ['AA', 'AC']});
+      })
+      .then((wooot)=> {


~~wooot~~ : -p

test/postgresql.test.js

jannyHou · 2020-05-02T05:13:20Z

test/postgresql.test.js

+        post.categories[1].should.eql('AB');
+        return Post.updateAll({where: {id: postId}}, {categories: ['AA', 'AC']});
+      })
+      .then((wooot)=> {


ditto, leaving it empty is ok :)

test/postgresql.test.js

jannyHou

@onaluf @spirAde Thank you for applying feedback so fast!

I took some time to verify the code on local and confirm the expected behavior. Your fix looks reasonable to me 👍 I left a few suggestions, most of them are just code refactor.

jannyHou · 2020-05-02T05:35:11Z

lib/postgresql.js

+    const isArrayDataType = prop.postgresql &&
+                            prop.postgresql.dataType &&
+                            (prop.postgresql.dataType.indexOf('[]') > -1);


@onaluf is varchar[] the only possible data type? or datatype in format *[] are all valid conditions?

If varchar[] is the only valid one, you can use

const isArrayDataType = prop.postgresql && prop.postgresql.dataType === 'varchar[]';

instead.

jannyHou · 2020-05-02T05:37:21Z

lib/postgresql.js

@@ -766,6 +766,26 @@ PostgreSQL.prototype.toColumnValue = function(prop, val, isWhereClause) {
    });
  }

+  if (prop.type instanceof Array) {


Nitpick: Array.isArray(prop.type) is better

jannyHou · 2020-05-02T05:37:46Z

lib/postgresql.js

+    const isArrayDataType = prop.postgresql &&
+                            prop.postgresql.dataType &&
+                            (prop.postgresql.dataType.indexOf('[]') > -1);
+    if (val instanceof Array) {


ditto Array.isArray(val) is better.

jannyHou · 2020-05-02T05:44:34Z

@slnode test please

onaluf · 2020-05-04T12:10:31Z

@jannyHou Thanks for the constructive feedback! I think I've solved all the point you raised.

jannyHou · 2020-05-04T18:12:14Z

@slnode test please

jannyHou

👍 @onaluf Thank you and cheers that all tests pass! The code LGTM.

jannyHou · 2020-05-05T02:14:19Z

@onaluf Almost there, could you squash the 2 commits into 1? Then we will help you land it!

onaluf · 2020-05-05T07:42:07Z

Done!

jannyHou · 2020-05-05T13:59:05Z

@slnode test please

jannyHou · 2020-05-05T15:01:21Z

@slnode test please

jannyHou · 2020-05-05T20:36:57Z

Merged 🎉 Thanks everyone again for the contribution and feedback!

* Fix serialization of arrays of string in update (#428) (Selim Arsever)

spirAde · 2020-05-07T20:45:11Z

@jannyHou @onaluf Thank you, guys!

Signed-off-by: SAMI BETTAYEB <sami3639@gmail.com>

* Fix serialization of arrays of string in update (loopbackio#428) (Selim Arsever) Signed-off-by: SAMI BETTAYEB <sami3639@gmail.com>

onaluf requested review from agnes512, dhmlau, emonddr and jannyHou as code owners March 20, 2020 09:08

onaluf force-pushed the string_array_field_serialization branch 2 times, most recently from ac706b9 to dc47d13 Compare April 25, 2020 21:32

bajtos added the community-contribution Patches contributed by community label Apr 27, 2020

jannyHou reviewed Apr 27, 2020

View reviewed changes

onaluf force-pushed the string_array_field_serialization branch from dc47d13 to 2e7a86d Compare April 27, 2020 22:31

onaluf force-pushed the string_array_field_serialization branch from 2e7a86d to e118c49 Compare April 28, 2020 07:21

onaluf commented Apr 28, 2020

View reviewed changes

jannyHou reviewed May 2, 2020

View reviewed changes

etodanik mentioned this pull request May 3, 2020

varchar[] doesn't work #434

Closed

dhmlau added this to the May 2020 milestone May 4, 2020

jannyHou approved these changes May 4, 2020

View reviewed changes

agnes512 approved these changes May 5, 2020

View reviewed changes

Fix serialization of arrays of string in update

f98144a

onaluf force-pushed the string_array_field_serialization branch from f5d80d6 to f98144a Compare May 5, 2020 07:38

jannyHou merged commit beedf56 into loopbackio:master May 5, 2020

jannyHou added a commit that referenced this pull request May 6, 2020

5.0.1

1246144

* Fix serialization of arrays of string in update (#428) (Selim Arsever)

agnes512 mentioned this pull request May 21, 2020

Applying filter for type Array field #441

Closed

bytewareteam mentioned this pull request Sep 28, 2020

Loopback filter on column "varchar []" does not work in PostgreSQL: #461

Closed

SAMIBETTAYEB pushed a commit to SAMIBETTAYEB/loopback-connector-postgresql that referenced this pull request Sep 2, 2021

Fix serialization of arrays of string in update (loopbackio#428)

007cdfa

Signed-off-by: SAMI BETTAYEB <sami3639@gmail.com>

SAMIBETTAYEB pushed a commit to SAMIBETTAYEB/loopback-connector-postgresql that referenced this pull request Sep 2, 2021

5.0.1

1c4d97d

* Fix serialization of arrays of string in update (loopbackio#428) (Selim Arsever) Signed-off-by: SAMI BETTAYEB <sami3639@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix serialization of arrays of string in update #428

Fix serialization of arrays of string in update #428

onaluf commented Mar 20, 2020

slnode commented Mar 20, 2020

achrinza commented Apr 12, 2020

onaluf commented Apr 12, 2020

jannyHou left a comment

jannyHou Apr 27, 2020

jannyHou Apr 27, 2020

jannyHou Apr 27, 2020

spirAde commented Apr 27, 2020 •

edited

Loading

onaluf commented Apr 27, 2020

onaluf commented Apr 27, 2020

onaluf commented Apr 28, 2020

onaluf Apr 28, 2020

jannyHou May 2, 2020

spirAde commented Apr 28, 2020 •

edited

Loading

jannyHou commented May 2, 2020

jannyHou May 2, 2020

jannyHou May 2, 2020

jannyHou left a comment

jannyHou May 2, 2020

jannyHou May 2, 2020

jannyHou May 2, 2020

jannyHou commented May 2, 2020

onaluf commented May 4, 2020

jannyHou commented May 4, 2020

jannyHou left a comment

jannyHou commented May 5, 2020

onaluf commented May 5, 2020

jannyHou commented May 5, 2020

jannyHou commented May 5, 2020

jannyHou commented May 5, 2020

spirAde commented May 7, 2020

Fix serialization of arrays of string in update #428

Fix serialization of arrays of string in update #428

Conversation

onaluf commented Mar 20, 2020

Checklist

slnode commented Mar 20, 2020

achrinza commented Apr 12, 2020

onaluf commented Apr 12, 2020

jannyHou 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

spirAde commented Apr 27, 2020 • edited Loading

onaluf commented Apr 27, 2020

onaluf commented Apr 27, 2020

onaluf commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spirAde commented Apr 28, 2020 • edited Loading

jannyHou commented May 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou 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

jannyHou commented May 2, 2020

onaluf commented May 4, 2020

jannyHou commented May 4, 2020

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou commented May 5, 2020

onaluf commented May 5, 2020

jannyHou commented May 5, 2020

jannyHou commented May 5, 2020

jannyHou commented May 5, 2020

spirAde commented May 7, 2020

spirAde commented Apr 27, 2020 •

edited

Loading

spirAde commented Apr 28, 2020 •

edited

Loading