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

Use input ident instead of associated type projections in ActiveModel derive #2349

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

svix-jplatte
Copy link
Contributor

Overview

Currently, sea-orm's ActiveModel derive generates impls like impl From<<Entity as sea_orm::EntityTrait>::Model> for ActiveModel. This prevents any other From<LocalType> for ActiveModel impls from being added from other crates that depend on the crate that defines the ActiveModel. This is a bug in rustc: rust-lang/rust#85898

This PR works around that by not using associated type projections for these impls. I couldn't figure out why this was done in the first place, as it seems like the real type this would resolve to would always be the input type of the derive macro. Maybe I'm wrong about that.

I found that the associated type projection was introduced in 890464f, and an example was updated but at the same time but I fail to see how it would be impacted by this change. Maybe CI will tell.

Changes

  • Simplify ActiveModel derive output to work around a compiler bug

@svix-jplatte
Copy link
Contributor Author

Gentle ping @tyt2y3

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2024

GitHub seems to have skipped the button to let us run CI

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2024

Now it's running

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2024

I found that the associated type projection was introduced in 890464f

thank you for digging this up. I have nearly forgotten the context. I think the original motivation is to use DeriveActiveModel outside of the normal entity definition, in which we'd have multiple ActiveModel sharing the same Model. But that use case has been replaced by DeriveIntoActiveModel soon after.

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2024

This prevents any other From for ActiveModel impls from being added from other crates that depend on the crate that defines the ActiveModel

@svix-jplatte can you also include a test case for this? I want to understand what did not compile for you, and be sure that we'd not break it in the future.

@svix-jplatte
Copy link
Contributor Author

This issue only manifests with multiple crates involved. Should I create two dummy crates that are just a regular part of the workspace and compiled as part of it? Where would I put them, tests/? They would be different from the other tests in that they're not actually test binaries, but individual cargo packages with their own Cargo.tomls.

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 15, 2024

Can you put the entity in https://github.com/SeaQL/sea-orm/tree/master/src/tests_cfg and then use it in integration tests?
I think the crate boundary would work here?

@svix-jplatte
Copy link
Contributor Author

I didn't know about that module, in that case I don't even need to define a new entity, an existing one using DeriveActiveModel works just fine. Pushed a test now, and verified that it fails to compile on master.

@svix-jplatte
Copy link
Contributor Author

Ping @tyt2y3 looks like CI has succeeded :)

@tyt2y3 tyt2y3 merged commit ead131d into SeaQL:master Oct 25, 2024
36 checks passed
@svix-jplatte svix-jplatte deleted the active-model-use-ident branch October 28, 2024 08:19
Copy link

github-actions bot commented Nov 4, 2024

🎉 Released In 1.1.1 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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