Skip to content

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Jan 6, 2022

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #384 (7b254c9) into main (73b1a2b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              main      #384    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          239       241     +2     
  Lines        12929     13135   +206     
==========================================
+ Hits         12929     13135   +206     
Impacted Files Coverage Δ
internal/orchestrator/orchestrator.go 100.00% <ø> (ø)
pkg/fftypes/jsonobject.go 100.00% <ø> (ø)
pkg/fftypes/message.go 100.00% <ø> (ø)
internal/apiserver/restfilter.go 100.00% <100.00%> (ø)
...nternal/apiserver/route_admin_get_config_record.go 100.00% <100.00%> (ø)
...nternal/apiserver/route_admin_put_config_record.go 100.00% <100.00%> (ø)
internal/apiserver/route_admin_put_config_reset.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_chart_histogram.go 100.00% <100.00%> (ø)
internal/apiserver/route_post_data.go 100.00% <100.00%> (ø)
internal/blockchain/fabric/fabric.go 100.00% <100.00%> (ø)
... and 28 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 73b1a2b...7b254c9. Read the comment docs.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor Author

Did a bit more improvement work, after exploratory UI testing during the development of hyperledger/firefly-ui#85

  • Support for matching partial hash/ID hex values
    • Meant treating the input as string, rather than requiring it to be a full parsable UUID
  • Being explicit about whether you are querying for a blank or null value
    • Tweak to the FIR to add a ? modifier to treat empty as nil in the code

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor Author

Found that SQLite doesn't support ILIKE, and needs a PRAGMA to be case sensitive.
Also that in PSQL ILIKE is slower than lower(field) LIKE '%search-in-lower%'

So made some tweaks to allow ILIKE to be disabled

@peterbroadhurst
Copy link
Contributor Author

Need to sort this in E2E that I didn't see in debug:

panic: sql: Register called twice for driver sqlite3_ff

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor Author

That should be solved.

Slightly more awkward revelation - we should be storing our JSON fields as TEXT not BYTEA I think, otherwise users can't perform a sub-string search. While not something people should rely on in production, it's a big pain if you can't as your operations console to tell you which data JSON contains the business info you need.

@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Jan 7, 2022

Good news it it seems our SQL layer can happily take []byte values from our jsonField type for both TEXT and BYTEA columns. So it should be the case that we can just change the migrations for new envs 🤞

@peterbroadhurst
Copy link
Contributor Author

I may have spoken too soon. Potentially we're now just storing hex in our TEXT field

@peterbroadhurst
Copy link
Contributor Author

ok - I've backed out the change from BYTEA -> TEXT.
Instead think I need to make it an error to query a partial match on a JSON field. This is something that the community might want to come back to (noting it would mean changing the internal type of fftypes.Byteable from []byte to string, and dealing with DB migration considerations).

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst marked this pull request as draft January 7, 2022 21:54
@peterbroadhurst
Copy link
Contributor Author

ok - big commit coming, so putting back to draft.

Also discussed with @nguyer that it might make sense for me to redirect this PR to the onchain-logic branch.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor Author

ok - good news #384 (comment) proved to be true on the database side, once I'd done the big code change on the FireFly side to use Go string (instead of []byte) as the type for the JSON types.

New envs will use TEXT and hence be happily able to do sub-string queries.

Existing envs will have BYTEA containing their JSON, and won't be able to query, but can still retrieve and store data.

@peterbroadhurst peterbroadhurst changed the title Rest API Query Syntax Enhancements FIR-4 (rich query updates) and FIR-7 (DX manifests) integration to onchain-logic branch Jan 12, 2022
@peterbroadhurst
Copy link
Contributor Author

Changing target... closing

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.

2 participants