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

feat(orm): codegen #11033

Merged
merged 26 commits into from
Jan 28, 2022
Merged

feat(orm): codegen #11033

merged 26 commits into from
Jan 28, 2022

Conversation

technicallyty
Copy link
Contributor

Description

adds orm code generation

Closes: #10737


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@github-actions github-actions bot added the C:orm label Jan 27, 2022
@aaronc aaronc mentioned this pull request Jan 27, 2022
19 tasks
@aaronc
Copy link
Member

aaronc commented Jan 27, 2022

We also need HasBy and GetBy for unique indexes other than the primary key. For instance, for ExampleTable u64,str, we should have:

HasByU64Str(u64 uint64, str string) (found bool, err error)
GetByU64Str(u64 uint64, str string) (*ExampleTable, err error)

@aaronc
Copy link
Member

aaronc commented Jan 27, 2022

Actually for constructors, I think we want to pass in ormdb.ModuleDB everywhere, ex:

func NewBalanceStore(db ormdb.ModuleDB) (BalanceStore, error) {
  table := db.GetTable(&Balance{})
  if table == nil {
    return nil, fmt.ErrorF("table %s not found", (&Balance{}).ProtoReflect().Descriptor().FullName())
  }
  return &balanceStore{table}, nil
}

func NewStateStore(db ormdb.ModuleDB) (StateStore, error) {
  balanceStore, err := NewBalanceStore(db)
  if err != nil {
    return nil, err
  }
  ...
}

@aaronc
Copy link
Member

aaronc commented Jan 27, 2022

@fdymylja I know you used pluralization in your codegen example on the file-level interfaces (i.e. Balance -> Balances()). I do miss something like this when trying to use this interface and we could use go-pluralize, but honestly it feels like a weird dependency for a stable API with all the custom rules it has (https://github.com/gertd/go-pluralize/blob/master/pluralize.go). How about we just generate BalanceStore(), etc.?

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

getting close!

orm/internal/testpb/test_schema.cosmos_orm.go Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This looks great and I think is basically done 👍. Just needs a little tidying up and then I think it's ready to go.

We can deal with pagination in a follow up and I think I want to clean up a little bit the Table interface under the hood, but that should also be in follow-ups.

orm/internal/bankexample/state.proto Outdated Show resolved Hide resolved
orm/internal/codegen/file.go Outdated Show resolved Hide resolved
orm/internal/codegen/index.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #11033 (d582846) into master (3a1027c) will increase coverage by 0.22%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11033      +/-   ##
==========================================
+ Coverage   65.86%   66.09%   +0.22%     
==========================================
  Files         644      691      +47     
  Lines       65630    69586    +3956     
==========================================
+ Hits        43229    45990    +2761     
- Misses      19920    20793     +873     
- Partials     2481     2803     +322     
Impacted Files Coverage Δ
orm/model/ormtable/index_impl.go 48.97% <ø> (ø)
orm/model/ormtable/primary_key.go 51.21% <ø> (ø)
orm/model/ormtable/unique.go 38.46% <ø> (ø)
server/config/config.go 40.31% <0.00%> (-0.32%) ⬇️
orm/model/ormtable/table_impl.go 54.50% <42.85%> (ø)
orm/internal/fieldnames/fieldnames.go 100.00% <100.00%> (ø)
orm/model/ormtable/build.go 77.43% <100.00%> (ø)
errors/abci.go 89.09% <0.00%> (ø)
errors/errors.go 85.04% <0.00%> (ø)
db/prefix/prefix.go 42.20% <0.00%> (ø)
... and 44 more

Copy link
Contributor

@fdymylja fdymylja left a comment

Choose a reason for hiding this comment

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

LGTM!

Two things (nonblocking):

  • I'd drop the suffix Store on bankStore.BalanceStore() - fine with it not being plural.
  • Need to add a note that in case state is split across multiple files the store interfaces set (bankstore) will not be generated correctly.

@aaronc
Copy link
Member

aaronc commented Jan 28, 2022

LGTM!

Two things (nonblocking):

  • I'd drop the suffix Store on bankStore.BalanceStore() - fine with it not being plural.

When I tried using it when it was just Balance it felt kind of weird 🤷

  • Need to add a note that in case state is split across multiple files the store interfaces set (bankstore) will not be generated correctly.

Why won't it be generated correctly?

@fdymylja
Copy link
Contributor

Why won't it be generated correctly?

If we have state1.proto and state2.proto: the store set will either generate state1 or state2, you could check all the files passed to protoc but this assumes state1.proto and state2.proto are generated at the same time.

@aaronc
Copy link
Member

aaronc commented Jan 28, 2022

Why won't it be generated correctly?

If we have state1.proto and state2.proto: the store set will either generate state1 or state2, you could check all the files passed to protoc but this assumes state1.proto and state2.proto are generated at the same time.

In this case you will end up with two file-level interfaces: State1Store and State2Store

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

tACK

Made a few commits on top of this to tidy up and tested locally.

Good work @technicallyty 👏

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 28, 2022
@mergify mergify bot merged commit 20b2605 into master Jan 28, 2022
@mergify mergify bot deleted the ty/orm-codegen branch January 28, 2022 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:orm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ORM codegen
3 participants