feat(orm): specify generic gRPC query service #11791

aaronc · 2022-04-26T21:53:22Z

Description

This proposes a generic gRPC query service that uses direct index access and can basically be implemented as a thin layer directly on top of ormtable.Table. There is relatively little code required to implement this but it can potentially provide a lot of value as a way to expose queries to basically everything that's using the ORM without needing to implement any custom gRPC queries.

A version of this which is type safe is also proposed in #11774 but that would require additional codegen of .proto files. #11774 also proposes a logical query layer instead of direct index access but that's substantially more code to do even naively and an optimized version would take more effort.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-04-26T22:11:40Z

Codecov Report

Merging #11791 (d84946a) into main (82412af) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #11791      +/-   ##
==========================================
+ Coverage   66.14%   66.18%   +0.03%     
==========================================
  Files         673      708      +35     
  Lines       71160    74055    +2895     
==========================================
+ Hits        47072    49016    +1944     
- Misses      21444    22127     +683     
- Partials     2644     2912     +268

Impacted Files	Coverage Δ
x/group/keeper/keeper.go	`54.43% <0.00%> (-0.43%)`	⬇️
orm/model/ormtable/unique.go	`44.09% <0.00%> (ø)`
orm/internal/fieldnames/fieldnames.go	`100.00% <0.00%> (ø)`
orm/encoding/ormfield/int64.go	`100.00% <0.00%> (ø)`
orm/encoding/ormkv/index_key.go	`62.82% <0.00%> (ø)`
orm/model/ormdb/file.go	`56.52% <0.00%> (ø)`
orm/encoding/ormfield/enum.go	`80.00% <0.00%> (ø)`
orm/model/ormtable/primary_key.go	`49.66% <0.00%> (ø)`
orm/model/ormtable/util.go	`57.89% <0.00%> (ø)`
orm/encoding/ormkv/primary_key.go	`57.77% <0.00%> (ø)`
... and 26 more

technicallyty

lgtm! exciting 💯

ValarDragon

This seems great to me!

Then JS libraries can build better direct interfaces on top of these ORM queries

robert-zaremba

left 2 suggestions

robert-zaremba · 2022-04-26T22:54:13Z

proto/cosmos/orm/v1alpha1/query.proto

+message GetResponse {
+
+  // result is the result of the get query. If no value is found, the gRPC
+  // status code NOT_FOUND will be returned.


what if a wrong index is used? I think we should return InvalidRequst in that case.

robert-zaremba · 2022-04-26T22:55:24Z

proto/cosmos/orm/v1alpha1/query.proto

+
+  // pagination is the pagination request.
+  cosmos.base.query.v1beta1.PageRequest pagination = 5;
+


how about adding reverse flag?

PageRequest already includes reverse

aaronc · 2022-04-26T22:58:37Z

I'd like to move this to cosmos.orm.query.v1 to version it separately. Any objections to that?

proto/cosmos/orm/v1alpha1/query.proto

aaronc

I'm actually thinking of simplying the API and removing IndexValue and maybe the query oneof, and using string (proto JSON encoding) instead.

I'm trying to think of gRPC gateway usage. Consider (the current format):

{"message_name":"cosmos.bank.v1.Balance","index":"address,denom","query":{"prefix":{"values":[{"str":"cosmos34sdgihi"}]}}}

vs

{"message_name":"cosmos.bank.v1.Balance","index":"address,denom","prefix":["cosmos34sdgihi"]}

Any objections?

aaronc · 2022-04-27T14:28:00Z

proto/cosmos/orm/query/v1/query.proto

+  // values are the values of the fields corresponding to the requested index.
+  // There must be as many values provided as there are fields in the index and
+  // these values must correspond to the index field types.
+  repeated IndexValue values = 3;


Suggested change

repeated IndexValue values = 3;

repeated string values = 3;

aaronc · 2022-04-27T14:28:53Z

proto/cosmos/orm/query/v1/query.proto

+  oneof query {
+
+    // prefix defines a prefix query.
+    Prefix prefix = 3;
+
+    // range defines a range query.
+    Range range = 4;
+  }


Suggested change

oneof query {

// prefix defines a prefix query.

Prefix prefix = 3;

// range defines a range query.

Range range = 4;

}

repeated string prefix = 3;

repeated string start = 4;

repeated string end = 5;

aaronc · 2022-04-28T20:11:56Z

I'm now thinking of maybe not taking this approach and instead going with logical queries, see #11774 (comment) and #11822

github-actions · 2022-06-13T00:04:59Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tac0turtle · 2022-06-13T18:23:31Z

@aaronc is it okay to merge this then come back?

aaronc · 2022-06-13T18:39:01Z

@aaronc is it okay to merge this then come back?

I'd want to mark it as alpha first

…rpc-proto

…nc/orm-grpc-proto

aaronc · 2022-06-13T20:34:00Z

Marked as v1alpha1 for now and going to merge. I think this may still be relevant as a very basic layer to sit under other tools as I mentioned here: #11822 (comment)

Mita23456

Fa
M

feat(orm): specify generic gRPC query service

6e1bb05

aaronc requested a review from alexanderbez as a code owner April 26, 2022 21:53

aaronc requested review from ryanchristo, technicallyty, robert-zaremba, amaury1093, fdymylja and tac0turtle April 26, 2022 21:54

aaronc mentioned this pull request Apr 26, 2022

ORM query service #11774

Open

aaronc requested a review from ValarDragon April 26, 2022 21:59

protogen

58789ee

technicallyty approved these changes Apr 26, 2022

View reviewed changes

ValarDragon approved these changes Apr 26, 2022

View reviewed changes

robert-zaremba approved these changes Apr 26, 2022

View reviewed changes

tac0turtle reviewed Apr 26, 2022

View reviewed changes

proto/cosmos/orm/v1alpha1/query.proto Outdated Show resolved Hide resolved

move to cosmos.orm.query.v1

b037e17

aaronc commented Apr 27, 2022

View reviewed changes

github-actions bot added the stale label Jun 13, 2022

Merge branch 'main' into aaronc/orm-grpc-proto

bef5caf

tac0turtle requested a review from a team as a code owner June 13, 2022 18:23

aaronc added 3 commits June 13, 2022 16:30

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/orm-g…

d7ba8a0

…rpc-proto

move to v1alpha1

fec53c5

Merge remote-tracking branch 'origin/aaronc/orm-grpc-proto' into aaro…

d84946a

…nc/orm-grpc-proto

aaronc added A:automerge Automatically merge PR once all prerequisites pass. C:orm and removed stale labels Jun 13, 2022

mergify bot merged commit d084887 into main Jun 13, 2022

mergify bot deleted the aaronc/orm-grpc-proto branch June 13, 2022 20:45

aaronc mentioned this pull request Feb 9, 2023

feat(orm)!: implement gRPC query support #14965

Closed

19 tasks

Mita23456 reviewed Dec 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orm): specify generic gRPC query service #11791

feat(orm): specify generic gRPC query service #11791

aaronc commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

technicallyty left a comment

ValarDragon left a comment

robert-zaremba left a comment

robert-zaremba Apr 26, 2022

aaronc Apr 26, 2022

robert-zaremba Apr 26, 2022

aaronc Apr 26, 2022

aaronc commented Apr 26, 2022

aaronc left a comment •

edited

Loading

aaronc Apr 27, 2022

aaronc Apr 27, 2022

aaronc commented Apr 28, 2022 •

edited

Loading

github-actions bot commented Jun 13, 2022

tac0turtle commented Jun 13, 2022

aaronc commented Jun 13, 2022

aaronc commented Jun 13, 2022

Mita23456 left a comment


		// pagination is the pagination request.
		cosmos.base.query.v1beta1.PageRequest pagination = 5;

feat(orm): specify generic gRPC query service #11791

feat(orm): specify generic gRPC query service #11791

Conversation

aaronc commented Apr 26, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Apr 26, 2022 • edited Loading

Codecov Report

technicallyty left a comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Apr 26, 2022

Choose a reason for hiding this comment

aaronc Apr 26, 2022

Choose a reason for hiding this comment

robert-zaremba Apr 26, 2022

Choose a reason for hiding this comment

aaronc Apr 26, 2022

Choose a reason for hiding this comment

aaronc commented Apr 26, 2022

aaronc left a comment • edited Loading

Choose a reason for hiding this comment

aaronc Apr 27, 2022

Choose a reason for hiding this comment

aaronc Apr 27, 2022

Choose a reason for hiding this comment

aaronc commented Apr 28, 2022 • edited Loading

github-actions bot commented Jun 13, 2022

tac0turtle commented Jun 13, 2022

aaronc commented Jun 13, 2022

aaronc commented Jun 13, 2022

Mita23456 left a comment

Choose a reason for hiding this comment

aaronc commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

aaronc left a comment •

edited

Loading

aaronc commented Apr 28, 2022 •

edited

Loading