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(core-api): always sort transactions by sequence and the requested field #2058

Merged
merged 8 commits into from Feb 5, 2019

Conversation

Projects
None yet
6 participants
@faustbrian
Copy link
Collaborator

faustbrian commented Feb 4, 2019

Proposed changes

Resolves #2041

Note: Currently the sequence order is ascending like the SPV, might be better to sort descending?

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Feb 4, 2019

@supaiku0 @air1one - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #2058 into develop will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2058      +/-   ##
===========================================
+ Coverage    73.76%   73.76%   +<.01%     
===========================================
  Files          367      367              
  Lines         8259     8261       +2     
  Branches      1187     1177      -10     
===========================================
+ Hits          6092     6094       +2     
+ Misses        2135     2134       -1     
- Partials        32       33       +1
Impacted Files Coverage Δ
packages/core-api/src/repositories/transactions.ts 82.89% <75%> (+0.22%) ⬆️

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 ccf3647...b7d8daf. Read the comment docs.

@zillionn

This comment has been minimized.

Copy link
Contributor

zillionn commented Feb 4, 2019

Currently the default order is descending and with this patch it'll be ascending, correct?

@@ -500,7 +500,9 @@ export class TransactionsRepository extends Repository implements IRepository {
return null;
}

public __orderBy(parameters): string[] {
public __orderBy(selectQuery, parameters): string[] {
selectQuery.order(this.query.sequence.asc);

This comment has been minimized.

@vasild

vasild Feb 4, 2019

Contributor

We would end up doing (in this order):

selectQuery.order(this.query.sequence.asc);
...
this._findManyWithCount(selectQuery, { ..., orderBy: [ "timestamp", "desc" ] });

I am not sure how this JS-to-SQL library works, but would it produce SQL ... ORDER BY id ASC, timestamp DESC or ... ORDER BY timestamp DESC, id ASC?

This comment has been minimized.

@vasild

vasild Feb 5, 2019

Contributor

This produces the following query:

SELECT "transactions"."block_id", "transactions"."serialized", "transactions"."timestamp" FROM "transactions" WHERE (("transactions"."sender_public_key" = $1) OR ("transactions"."recipient_id" = $2)) ORDER BY "transactions"."sequence" DESC, "transactions"."timestamp" DESC OFFSET 0 LIMIT 5}

which is probably not what the intention was, and is definitely not returning results sorted by timestamp, even though ...&orderBy=timestamp:desc is present in the URL.

For example:

SELECT timestamp, sequence FROM transactions WHERE sender_public_key = '0219c48e4b9c566b9ed26ad8f762e1fcf425c72776fce61e4839705dd35bfd43dd'
ORDER BY sequence DESC, timestamp DESC;
 timestamp | sequence 
-----------+----------
  49221058 |        8
  49220498 |        7
  49220778 |        5
  49225368 |        4
  49219410 |        4
  49226538 |        3
  49225714 |        3
  49226154 |        1
  49225058 |        1
  49224890 |        1
  49126378 |        1
  49104538 |        1
SELECT timestamp, sequence FROM transactions WHERE sender_public_key = '0219c48e4b9c566b9ed26ad8f762e1fcf425c72776fce61e4839705dd35bfd43dd'
ORDER BY timestamp DESC, sequence DESC;
 timestamp | sequence 
-----------+----------
  49226538 |        3
  49226154 |        1
  49225714 |        3
  49225368 |        4
  49225058 |        1
  49224890 |        1
  49221058 |        8
  49220778 |        5
  49220498 |        7
  49219410 |        4
  49126378 |        1
  49104538 |        1
(12 rows)

This comment has been minimized.

@faustbrian

faustbrian Feb 5, 2019

Author Collaborator

Submit a PR to improve it rather then simply commenting.

This comment has been minimized.

@vasild

vasild Feb 5, 2019

Contributor

Will do.

However, why did you merge a flawed patch in the first place? It is a good practice to look into and resolve all review comments before merging a pull request.

@supaiku0 supaiku0 self-requested a review Feb 4, 2019

supaiku0 added some commits Feb 4, 2019

@faustbrian faustbrian merged commit 0318bd8 into develop Feb 5, 2019

6 checks passed

ci/circleci: test-node10-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-2 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-2 Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the fix/api-sort branch Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment