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

feat: support array within array #1699

Merged
merged 1 commit into from
Mar 19, 2019
Merged

feat: support array within array #1699

merged 1 commit into from
Mar 19, 2019

Conversation

hacksparrow
Copy link
Contributor

Description

POST request body:

 { items": [
    [
      {"name": "APPLE"},
      {"serial": 67}
    ]
  ]
}

Currently stored as:

 { "items": [
    {
      "0": {
        "name": "APPLE"
      },
      "1": {
        "serial": 67
      }
    }
  ]
}

Will be stored as:

 { items": [
    [
      {
        "name": "APPLE"
      },
      {
        "serial": 67
      }
    ]
  ]}

Related issues

strongloop/loopback#4132

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@hacksparrow
Copy link
Contributor Author

@raymondfeng @bajtos please review.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

I think this will not handle the case where the nested items need to be converted from plain data objects into model instances, but that can be fixed later. The change proposed here is a good incremental improvement.

Please get at least one or two more approvals before landing. I am not very familiar with juggler code base, I may have missed a problem that's not obvious.

@hacksparrow
Copy link
Contributor Author

Agreed. I am aware that there can be other conditions when the resulting object will be unexpected. Let's just focus on the reported bug at the moment.

if (isClass(this.itemType)) {
return new this.itemType(item);
} else {
if (Array.isArray(item)) return item;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to test if item instanceof List to convert List, which is a special array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case it is breaking some test cases. The data structure which elicited the bug is a very unlikely one. This PR takes care of that scenario without side effects.

I think we should land the PR and move on.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@hacksparrow hacksparrow merged commit 3f9448f into master Mar 19, 2019
@rmg rmg deleted the fix/list-format branch March 18, 2021 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants