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

Allow Ecto.UUID PKs #129

Closed
wants to merge 1 commit into from

Conversation

vinniefranco
Copy link

Adds a compile-time config of :ecto_primary_key_type allowing use optional use of Ecto.UUID

@tompave
Copy link
Owner

tompave commented Mar 14, 2022

Thank you for opening the PR.

Can you explain why you think this is required? In other words, why do you need a UUID PK in the FWF table?

@vinniefranco
Copy link
Author

Hi @tompave

Great question! So, the reason I need UUID-based PKs is due to the way our production environment is set up. Int based pk/fk are a no-go

@tompave
Copy link
Owner

tompave commented Mar 15, 2022

I see, that makes sense, but I'm not sure this is the right way to go about it.
There is an open issue for how config is referenced in the ecto record file (#123), and this change would suffer from the same issue.

I'd be happy to accept a PR to fix the problem described in that issue, and then a second PR to add this new functionality using the correct compile-time config approach.

@vinniefranco
Copy link
Author

vinniefranco commented Mar 15, 2022

I see, that makes sense, but I'm not sure this is the right way to go about it. There is an open issue for how config is referenced in the ecto record file (#123), and this change would suffer from the same issue.

I'd be happy to accept a PR to fix the problem described in that issue and then a second PR to add this new functionality using the correct compile-time config approach.

Yeah, I'm not sure that's the correct approach. put_meta is for already built structs, so while one could dynamically change the source right before an update/insert/delete, it appears one cannot dynamically change the primary key definitions at run-time especially when the primary key is configured using a compile-time module attribute.

It would be better to name and document what is explicitly compile-time and run-time, as I've run into issues configuring FWF between ct configs/<env.exs> and the rt release.exs with Mix.

I'm happy to help document the potential gotchas if need be

@tompave
Copy link
Owner

tompave commented Mar 15, 2022

Yeah, I'm not sure that's the correct approach.

Which one? The one in your PR, or what's discussed in that issue?

put_meta is for already built structs,

Yes, I don't think the suggestion in the opening message of that issue is the right one. I'm suggesting a different approach in my replies.

It would be better to name and document what is explicitly compile-time and run-time, as I've run into issues configuring FWF between ct configs/<env.exs> and the rt release.exs with Mix.
I'm happy to help document the potential gotchas if need be

There were some gotchas, but they (or most of them) have been solved with the adoption of Application.compile_env. If you've run into issues configuring FWF in different envs I'd like to know if it was with an older version or with the latest. Chances are that you wouldn't have issues in the latest.

@vinniefranco
Copy link
Author

Yes, I don't think the suggestion in the opening message of that issue is the right one. I'm suggesting a different approach in my replies.

Ahh, yes I see that. My apologies

@tompave
Copy link
Owner

tompave commented Mar 20, 2022

With regard to my message above (#129 (comment)), that has now been fixed with #130. If you still want to update this PR you can adopt that pattern.

@tompave
Copy link
Owner

tompave commented Sep 11, 2022

Closing because of inactivity.

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