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

[Chore] Refactor Lemmy API common to avoid exposing structs directly from DB #5457

Open
5 tasks done
SleeplessOne1917 opened this issue Feb 27, 2025 · 7 comments
Open
5 tasks done
Assignees
Labels
area: api enhancement New feature or request

Comments

@SleeplessOne1917
Copy link
Member

Requirements

  • Is this a feature request? For questions or discussions use https://lemmy.ml/c/lemmy_support
  • Did you check to see if this issue already exists?
  • Is this only a feature request? Do not put multiple feature requests in one issue.
  • Is this a backend issue? Use the lemmy-ui repo for UI / frontend issues.
  • Do you agree to follow the rules in our Code of Conduct?

Is your proposal related to a problem?

Working on the rust client and leptos UI, I noticed that several types are pulled directly from the re-exported lemmy_db_schema crate. At the very least, it would be good to re-export them in relevant modules in lemmy_api_common instead.

More importantly, if lemmy_api_common is only meant to be types of requests, responses, and shapes used in them, them it is important that we don't leak too much funtionality from the rest of the app through this library. This has become a problem working on the leptos UI: the dependency on uuid is leaking through, and a recent update to uuid is causing compilation to fail when targetting WASM, even when using the crate at version 0.19.8.

Describe the solution you'd like.

Refactor lemmy_api_common to expose as little logic (and as few dependencies) to downstream projects as possible.

Describe alternatives you've considered.

Add a config gate to lemmy_db_schema's Cargo.toml to make the uuid dependency use a valid source of randomness when targetting WASM. I'm not a fan of this idea, as I really don't think a crate used for the app's database that is solely used in the server should need to worry about WASM support.

Make a new crate (lemmy_models or something to that effect) with the barest information available and tell people to use that instead of lemmy_api_common.

Additional context

No response

@SleeplessOne1917 SleeplessOne1917 added area: api enhancement New feature or request labels Feb 27, 2025
@SleeplessOne1917 SleeplessOne1917 self-assigned this Feb 27, 2025
@Nutomic
Copy link
Member

Nutomic commented Feb 27, 2025

So you mean adding pub use uuid; etc in lemmy_api_common? That sounds fine.

@dessalines
Copy link
Member

dessalines commented Feb 27, 2025

api common already has a uuid feature flag that you should be able to turn off.

It looks like we just also need to make that a feature flag for db_schema.

rg uuid crates/db_schema/

It also looks like that's only being used for the captcha_answer. That should just be made into a string anyway.

@Nutomic
Copy link
Member

Nutomic commented Feb 28, 2025

That would require a db migration because captcha_answer has a column with type uuid and default gen_random_uuid() which we are using (instead of generating the uuid in Rust).

@dessalines
Copy link
Member

Yep. Looks like there's only two uuid columns, in the captcha table, and the jwt_secret.

We'd need to alter the columns, then use postgres' uuid_to_string function after generating the new uuid for the new column definition.

https://hatchjs.com/postgres-cast-uuid-to-string/

Its up to @SleeplessOne1917 if that's worth necessary. Ideally wasm should just support UUID.

@SleeplessOne1917
Copy link
Member Author

Starting to work on this. Is there a command that needs to be run to generate a migration? The migration folders look like they were named programmatically.

Also,

Ideally wasm should just support UUID.

Currently UUID can be used with WASM, but it can only be enabled if either the "js", "rng-getrandom", or "rng-rand" feature are enabled.

@dessalines
Copy link
Member

diesel migration generate NAME

Do those add a lot of bulk?

UUID seems like a basic enough thing for a front end to have anyway. uuid-rs/uuid#350 (comment)

@SleeplessOne1917
Copy link
Member Author

Do those add a lot of bulk?

UUID itself doesn't, however refactoring the code so that we can use what are basically just API models without pulling in any of the DB stuff proved to be a much more difficult and labor-intensive refactor than I anticipated. It would be nice to be able to use all of the API types without having a peer dependency on diesel, for instance.

UUID seems like a basic enough thing for a front end to have anyway.

In this case, the difficulty come from direct use of a struct meant to be used in the data layer of the backend application. This issue explains the problem. Essentially, recent versions of uuid use a new major version of getrandom, which changes how it's handled when targeting WASM. Dependents of the uuid dependency now need to opt-in to certain features to use the crate in WASM: js, rng-getrandom, or rng-rand. Since the DB types are returned as part of the response and lemmy_db_schema doesn't use any of those features, it breaks things when trying to use those types in a WASM context even if you're not generating new UUIDs (and hence don't need a source of randomness).

I'll see if there's a way to tweak the dependencies to make it so things don't break when targetting WASM. A big part of the issue is that uuid is both a direct and indirect dependency (lemmy_api_common depends on infer, which depends on cfb, which depends on uuid). I'll also ask around the Leptos community to see if any of them have figured out how to resolve this issue without modifying upstream code. If the refactor ends up being the only viable option, I'll have to do some reading on design and refactoring large codebases before attempting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants