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

refactor: id prefixes #1546

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor: id prefixes #1546

wants to merge 1 commit into from

Conversation

turip
Copy link
Contributor

@turip turip commented Sep 20, 2024

Overview

This change makes sure that going forward all IDs generated will have ID prefixes, causing easier identification of the entity in question.

The old data is not modified.

Notes for reviewer

This change makes sure that going forward all IDs generated will have
ID prefixes, causing easier identification of the entity in question.

The old data is not modified.
@turip turip added release-note/breaking-change Release note: Breaking Changes release-note/feature Release note: Exciting New Features labels Sep 20, 2024
@@ -169,3 +196,11 @@ func (c CustomerAddressMixin) Fields() []ent.Field {
field.String(fmt.Sprintf("%s_address_phone_number", c.FieldPrefix)).Optional().Nillable(),
}
}

func Must(m ent.Mixin, err error) ent.Mixin {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we're already using it then i suggest maybe lo.Must

Comment on lines +3 to +7
// Table of ID prefixes for each entity type.
// Please make sure you are picking a unique prefix for each entity type.
// Recommendation:
// - at least 3 characters of the entity type name.
// - max length is 8 characters (field size limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is the hard part to get right. I'm not 100% scoping is the way to go, I'd rather define what to do in case of name collisions and use as short names as possible... wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, scoping is 100% TBD. Initially I was doing stuff like

  • BillingInvoiveItem => bii_
  • Customer => c_

But then I didn't like the one-character names, so that's why I ended up with this scoping thing. I am not against dropping that part.

I would just mandate that there are no collisions, as seven characters should be enough.

}

func (i idMixin) ULIDWithPrefix() string {
return fmt.Sprintf("%s_%s", i.IDPrefix, ulid.Make().String())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: With the current IDs the _ is a duplicate.

Base automatically changed from fix/key-mixin-indexing to main September 20, 2024 10:47
-- modify "billing_invoice_items" table
ALTER TABLE "billing_invoice_items" ALTER COLUMN "id" TYPE character varying(34), ALTER COLUMN "invoice_id" TYPE character varying(34);
-- modify "billing_invoices" table
ALTER TABLE "billing_invoices" ALTER COLUMN "id" TYPE character varying(34), ALTER COLUMN "billing_profile_id" TYPE character varying(34), ALTER COLUMN "workflow_config_id" TYPE character varying(34);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: we need to change the schema override too (or remove in case of edge fields as it seems like ent autogenerates the types)

@GAlexIHU
Copy link
Contributor

@turip I believe we agreed we'd postpone the id prefixing for the time being, and once we do it would only affect new IDs.

Would you be fine with closing this PR, or is this still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/breaking-change Release note: Breaking Changes release-note/feature Release note: Exciting New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants