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

fix: correctly initialize Array when values contain a single integer element #6235

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

iamdavidmartin
Copy link
Contributor

Issue #, if available:

Description of changes:
When a request to an external data source returns a single value that is of type number, the array is incorrectly initialized. This scenario means that all requests to ElasticSearch with results sorted by date fail.

For example, when querying a locally running ElasticSearch instance with results sorted by date, the date is always returned is in milliseconds: 1608657312744. Since that value is a number, the call to

super(...values)

becomes

super(1608657312744)

Which tries to initialize an array of length 1608657312744. See the documentation: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Array

A JavaScript array is initialized with the given elements, except in the case where a single argument is passed to the Array constructor and that argument is a number (see the arrayLength parameter below). Note that this special case only applies to JavaScript arrays created with the Array constructor, not array literals created with the bracket syntax.

This pull request correctly initializes the array when the length is 1.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iamdavidmartin iamdavidmartin requested a review from a team as a code owner December 22, 2020 17:24
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #6235 (c06e7d9) into master (473bea5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #6235    +/-   ##
========================================
  Coverage   57.05%   57.06%            
========================================
  Files         468      468            
  Lines       21489    21492     +3     
  Branches     4273     4060   -213     
========================================
+ Hits        12261    12264     +3     
- Misses       8349     8369    +20     
+ Partials      879      859    -20     
Impacted Files Coverage Δ
...psync-simulator/src/velocity/value-mapper/array.ts 97.43% <100.00%> (+0.21%) ⬆️
packages/amplify-util-mock/src/api/api.ts 0.00% <0.00%> (ø)
packages/graphql-mapping-template/src/print.ts 35.29% <0.00%> (ø)
packages/amplify-util-mock/src/storage/storage.ts 0.00% <0.00%> (ø)
...ges/amplify-util-mock/src/CFNParser/stack/index.ts 0.00% <0.00%> (ø)
...es/amplify-util-mock/src/api/resolver-overrides.ts 0.00% <0.00%> (ø)
...es/graphql-transformer-core/src/stripDirectives.ts 35.29% <0.00%> (ø)
.../amplify-cli-core/src/state-manager/pathManager.ts 67.85% <0.00%> (ø)
.../amplify-util-mock/src/utils/lambda/loadMinimal.ts 0.00% <0.00%> (ø)
...graphql-transformer-core/src/TransformerContext.ts 25.09% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 206ab32...c06e7d9. Read the comment docs.

@jhockett jhockett changed the title Correctly initialize Array when values contain a single integer element fix: correctly initialize Array when values contain a single integer element Jan 4, 2021
@jhockett jhockett added the pending-review Pending review from core-team label Jan 4, 2021
Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

@iamdavidmartin thanks for the fix, nice catch!

@attilah attilah merged commit c410678 into aws-amplify:master Jan 7, 2021
@github-actions
Copy link

github-actions bot commented Jan 8, 2022

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pending-review Pending review from core-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants