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

DefaultCrudRepository#update stores invalid representation of array-typed property in PostgreSQL (Error: could not create List from JSON string) #2532

Closed
chadmg opened this issue Mar 4, 2019 · 14 comments

Comments

@chadmg
Copy link

chadmg commented Mar 4, 2019

Description

My model looks like this:

@model()
export class Order extends Entity {
  @property({
    type: 'string',
    id: true,
    required: true,
  })
  id: string;

  @property({
    type: 'array',
    itemType: Item
  })
  items: Array<Item>;

  @property({
    type: 'date',
    required: true,
  })
  date: string;

  constructor(data?: Partial<Order>) {
    super(data);
  }
}

@model()
export class Item extends Model {

  @property({
    type: 'string',
    required: true,
  })
  sku: string;

  @property({
    type: 'string',
    required: true,
  })
  name: string;

When I create an Order entity using a repository:

this.orderRepository.create(order)

It stores the items array properly (in text column):

[{"sku":"69434","name":"Bubblegum"}]

However when I try to update an Order that already exists:

this.orderRepository.update(order)

The items array ends up being stored like this in the text column:

{"{\"sku\":\"69434\",\"name\":\"Bubblegum\"}"}

Then when I try to retrieve orders I get this error:

Error: could not create List from JSON string: "{\"{\\\"sku\\\":\\\"69434\\\",\\\"name\\\":\\\"Bubblegum\\\"...

Current Behavior

Array property is being set to invalid string on update.

Expected Behavior

Update should treat array property same as create method does and store valid JSON.

@dhmlau
Copy link
Member

dhmlau commented Mar 4, 2019

@chadmg, can you define the items as something like this:

@property.array(ShoppingCartItem, {required: true})
  products: ShoppingCartItem[];

See https://github.com/strongloop/loopback4-example-shopping/blob/master/src/models/order.model.ts#L29 for examples.

@dhmlau dhmlau self-assigned this Mar 4, 2019
@chadmg
Copy link
Author

chadmg commented Mar 4, 2019

@dhmlau Thanks for the reply! This didn't seem to change anything. Problem still occurs.

Note: It only happens when using DefaultCrudRepository#update

As a workaround I've replaced update with calls to delete then create and everything is working as expected.

@chadmg
Copy link
Author

chadmg commented Mar 6, 2019

@dhmlau Any update on this? Seems like a pretty critical issue with Loopback if a simple update operation is storing invalid and unretrievable data in the DB.

I don't think I'm doing anything wrong - please let me know if I am - but I don't understand how this isn't an issue for other users?

@dhmlau
Copy link
Member

dhmlau commented Mar 7, 2019

Hi @chadmg , sorry about the delay. Let me run your scenario tomorrow and try the update.

@dhmlau
Copy link
Member

dhmlau commented Mar 7, 2019

@chadmg, I was able to reproduce your problem.
I initially was using the API Explorer to test and realize that the PUT is using orderRepository.replaceById not orderRepository.updateById as you mentioned.

@bajtos, would it be part of the PATCH spike that you're looking into?

@chadmg
Copy link
Author

chadmg commented Mar 7, 2019

@dhmlau To provide some more details, through testing I've found this issue happening with both DefaultCrudRepository#update and DefaultCrudRepository#updateById.
Doing either a replaceById or calling both deleteById and create work correctly.

Side question: Where are all of the docs for these methods? I'm not even sure what the difference between update and updateById are... it seems like loopback 4 docs are completely lacking on the persistence/ORM layer.

@dhmlau
Copy link
Member

dhmlau commented Mar 13, 2019

@chadmg , thanks for the info. Yes, we're aware of the limitation for PATCH. As the first step, a spike is in progress for the support. Please join the discussion in: #2082.

The API docs can be found in http://apidocs.loopback.io/. Unfortunately, looking at DefaultCrudRepository, it doesn't have much information.

If you agree, I'd like to continue the discussion of the PATCH support in #2082, and we'll use this issue to see how we can improve our API docs. Contributions are welcomed!

@marioestradarosa
Copy link
Contributor

@chadmg ,

upateById under the hood is calling an updateAll method from the model legacy juggler, it creates a where clause based on the id and execute the operation to update all records in the database that matches the criteria, this is dangerous since we can ended up updating all the table rows.

replaceByID on the other hand just proxies the model replaceById method ensuring the operation gets executed on only one record/instance of the model/table. This is safer since that's what we usually supposed to do update one record

The generated controller @put('/myModels/{id}' invokes safely the replaceById mentioned above and internally in the similar way to the create method, it casts the model instance to DataObject<T> which honors partial content. Both create and updatebyId uses this DataObject, thus the current generated controller should work as expected in your case, as you mentioned replaceById works.

@dhmlau
Copy link
Member

dhmlau commented Mar 19, 2019

@marioestradarosa, thanks. Would you like to submit a PR to improve our API docs? Thanks.

@dhmlau dhmlau added the Docs label Mar 19, 2019
@dhmlau dhmlau removed their assignment Mar 19, 2019
@marioestradarosa
Copy link
Contributor

@dhmlau ok, I will do work on it this weekend 👍

@AsjadMahmood
Copy link

AsjadMahmood commented Sep 16, 2019

Any update on this issue. I am having the exact same issue when updating through path request results the object array to be stored like this :

{"{\"store_id\":\"db02b3b8-445c-46ae-90f1-f57f891c709a\",\"store_name\":\"s5\"}","{\"store_id\":\"188273ca-0e66-4c55-a189-bc73d6d4a121\",\"store_name\":\"s6\"}"}

it works fine when creating the data but while updating it creates problems

@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Sep 20, 2020
@achrinza achrinza removed the stale label Sep 20, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants