8000 use strconv.FormatInt for string id by stevenferrer · Pull Request #634 · ent/ent · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

use strconv.FormatInt for string id #634

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

Closed

Conversation

stevenferrer
Copy link

Use strconv.FormatInt instead of type casting string ID

@facebook-github-bot
Copy link
Contributor

Hi @stevenferrer!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 22, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@stevenferrer stevenferrer changed the title use strconv.FormatInt instead for string id use strconv.FormatInt for string id Jul 22, 2020
@a8m
Copy link
Member
a8m commented Jul 23, 2020

Hey @stevenferrer and thanks for your contribution.

Can you give more context for this PR? I assume it's related to previous functionality mentioned here - #127 (comment)

@stevenferrer
Copy link
Author

Hi @a8m, likewise, thank you very much for this amazing project!

I just want to address a small issue when using --idtype string specifically a line generated in assignValues and [Entity]Create.sqlSave.

The difference demonstrated below:

Current: fmt.P 8000 rintf("%#v", string(1)) produces "\x01"
Proposed: fmt.Printf("%#v", strconv.FormatInt(1, 10) produces "1"

I'm not sure though if #127 is related.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@a8m
Copy link
Member
a8m commented Jul 23, 2020

Can you elaborate how do you use string id-type with SQL? IDs are stored as int in the database? If yes, what's the reason for this?

As mentioned in the linked comment:

This (legacy) option was added to allow using the same generate ent package for both AWS Neptune (gremlin) and SQL dialects. Neptune uses UUID strings for its IDs, and MySQL is configured with numeric types for its PKs (in our deployment). Therefore, the type in the code must be string, and we convert it to int before/after the data were loaded.

@stevenferrer
Copy link
Author

An example is when you use json.Marshal:

brown := client.User.Create().SetName("brown").SaveX(ctx)
b, _ := json.Marshal(brown)
fmt.Println(string(b))

Outputs:

{"id":"\u0001","name":"brown"}

@a8m
Copy link
8000 Member
a8m commented Jul 23, 2020

Thanks for sharing the example @stevenferrer. Can you please answer my previous comment to help me get more context?

Can you elaborate how do you use string id-type with SQL? IDs are stored as int in the database? If yes, what's the reason for this?

@stevenferrer
Copy link
Author

We're using PostgreSQL and before migrating some other parts of our system to ent, we're already using bigserial as our ID type and we're mapping it as string in our Go structs.

For example, the below table:

CREATE TABLE address (
    id bigserial NOT NULL,
    unit_no varchar NOT NULL,
    barangay varchar NOT NULL,
    province varchar NOT NULL,
    city varchar NOT NULL,
    updated_at timestamp,
    created_at timestamp NOT NULL DEFAULT now()
);

is mapped to this struct:

type Address struct {
	ID string `json:"id"`
	UnitNo string `json:"unitNo"`
	Barangay string `json:"barangay"`
	Province string `json:"province"`
	City string `json:"city"`
	UpdatedAt *time.Time `json:"-"`
	CreatedAt *time.Time `json:"createdAt"`
}

@a8m
Copy link
Member
a8m commented Jul 23, 2020

We're using PostgreSQL and before migrating some other parts of our system to ent, we're already using bigserial as our ID type and we're mapping it as string in our Go structs.

Thanks for sharing this information.

There's no way to auto-map numeric value to string type in ent. Your PR fixes one problem with this, but there are so many other places that need to be changed in order to support this.

The GoType option is used exactly for cases like this, but it doesn't support ID fields as the moment. Once I'll add the support for this, you will be able to use it as follows:

type StringID string // Need to implement the schema/field.ValueScanner interface.

func (User) Fields() []ent.Field {
	return []ent.Field{
		field.Int("id").
			GoType(new(StringID)),
	}
}

If you don't want to be blocked on this, you can use external-templates for now (to override the existing behavior) and update your code in the future.

@stevenferrer
Copy link
Author

Ok, got it. Will probably use a workaround for now, thanks for the suggestion! Closing this PR then.

@a8m
Copy link
Member
a8m commented Jul 23, 2020

I'll update this PR with the new API soon @stevenferrer.
Thanks for your contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0