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

POC use ent as dbv2 #2940

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

POC use ent as dbv2 #2940

wants to merge 3 commits into from

Conversation

zwpaper
Copy link
Contributor

@zwpaper zwpaper commented Feb 8, 2024

I have divided the PR into 3 commits to show how ent could be used.

  1. c764867 define the db schema, memo, memo_relation
  2. 2d4ae64 ent auto-generate the db client codes
  3. ef5b193 add a dbv2 to store and use it for get memo and memo relations.

@zwpaper
Copy link
Contributor Author

zwpaper commented Feb 8, 2024

I mark it as draft, and we can discuss it here.

the PR should actually work for API v2 to get memos.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe memo_relation

tempList, err := s.Store.ListMemoRelations(ctx, &store.FindMemoRelation{
MemoID: &request.Id,
})
tempList, err := s.Store.V2.MemoRelation.
Copy link
Member

Choose a reason for hiding this comment

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

Should we wrap a method in the store package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's also my first try wrapping the db inside store.

but there are a few concerns, and we can discuss:

  1. using the origin store interface has no way to distinguish v1 or v2, we can only replace the origin v1 directly in each function step by step.
  2. it seems not worth creating a new store interface only for ent operations
  3. the ent client seems to be easy enough to use

@boojack
Copy link
Member

boojack commented Feb 19, 2024

Sorry for the late review. Overall, I think it is a good try!

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