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.

Sign up for GitHub

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

Jump to bottom

Add pending and rejected fields to messages, and use for clear sort order #76

Merged
merged 3 commits into from
Jun 15, 2021
Merged

Add pending and rejected fields to messages, and use for clear sort order #76

merged 3 commits into from
Jun 15, 2021

Conversation

peterbroadhurst
Copy link
Contributor

  • Fixes this error seen after groups were added:
    "error": "FF10121: Database resultset read error from table 'messages': sql: Scan error on column index 8, name \"group_hash\": encoding/hex: invalid byte: U+0020 ' '"
    • The problem here was .Set("group", code incorrectly using a non-nil string for nil values
    • This was because all hashes were using StringField in the QueryFactory implementations
    • Created a Bytes32Field as a partner to UUIDField and swapped them all over
  • Fixes Sort messages by confirmed, not the created sequence #72 "Sort messages by confirmed, not the created sequence"
    • This was a little harder than I expected.
    • The absolute order will always be the /events sequence (because clocks can jitter in exceptional circumstances), but it is very strange if the order of the messages in the /messages collection (and hence the UI) does not match the order you were delivered them
    • To get an order that makes sense I had to add two fields to messages:
      • pending - this starts true, and goes to false once the message is confirmed
      • rejected - is set to true if a message is confirmed, but with a failure (partners a message_rejected event)
    • This was necessary so that I could have a sort order of:
      • pending,confirmed,created (all descending) - meaning pending messages are always listed first in created order, then confirmed messages in order - including failed confirmed messages
    • I renamed message_invalid to message_rejected for consistency
    • There's some faff as PSQL and QL differ on a couple of fundamentals
      • QL does not support sorting bool columns - so I went for a smallint approach for the pending column
      • QL does not support per-column sorting - so I went for a consistent sort order
    • I also made it so you can specify a - prefix on individual column sorts on the API
  • Small tweak to the e2e test to use the -n option on ff start proposed in Suggested improvements for local dev firefly-cli#37

@peterbroadhurst peterbroadhurst requested a review from nguyer June 15, 2021 04:54
@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2021 •
edited
Loading

Codecov Report

Merging #76 (3cd48b3) into main (ddd824f) will increase coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   98.83%   99.38%   +0.55%     
==========================================
  Files         168      169       +1     
  Lines        7909     8003      +94     
==========================================
+ Hits         7817     7954     +137     
+ Misses         75       45      -30     
+ Partials       17        4      -13
Impacted Files Coverage Δ
pkg/fftypes/event.go 100.00% <ø> (ø)
internal/apiserver/restfilter.go 100.00% <100.00%> (ø)
internal/database/postgres/postgres.go 100.00% <100.00%> (ø)
internal/database/ql/ql.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/batch_sql.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/config_record_sql.go 100.00% <100.00%> (+41.77%) ⬆️
internal/database/sqlcommon/data_sql.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/datatype_sql.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/event_sql.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/filter_sql.go 100.00% <100.00%> (ø)
... and 22 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 ddd824f...3cd48b3. Read the comment docs.

@peterbroadhurst
Fix missed coverage
3cd48b3 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst peterbroadhurst mentioned this pull request Jun 15, 2021
Merged
@nguyer nguyer merged commit 3cd48b3 into hyperledger:main Jun 15, 2021
peterbroadhurst added a commit that referenced this pull request Sep 26, 2023
@peterbroadhurst
Merge pull request #76 from kaleido-io/manifest-fix
798c472 
Fix merge issue in manifest.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nguyer nguyer Awaiting requested review from nguyer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sort messages by confirmed, not the created sequence
3 participants
@peterbroadhurst @codecov-commenter @nguyer