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

/* integer */ comments seemingly aren't applied accurately #190

Open
TheBrokenRail opened this issue Sep 26, 2023 · 15 comments
Open

/* integer */ comments seemingly aren't applied accurately #190

TheBrokenRail opened this issue Sep 26, 2023 · 15 comments

Comments

@TheBrokenRail
Copy link
Contributor

So, #188 was just merged to hopefully make determining which numbers were meant to be integers easier. But now I'm even more confused.

At first glance, my assumption was anything with a /* integer */ comment was meant to be a integer and everything else was floating-point. But types like CommentId and CommunityId do not have this comment. And I highly doubt that CommunityId is supposed to be floating-point.

So my options are:

  1. Assume everything is an integer. This would work for the most part (and is what I'm doing now), but would break for types that are actually floating-point.
  2. Assume everything with the /* integer */ comment is an integer and everything else is floating-point. This would also work for the most part, except it will treat exceptions like CommunityId as floating-point and IMO that's a bad idea.
  3. Assume everything is floating-point. This would work (I mean, it's what JS does), but it would throw away some of the advantages of static typing and just be a bad idea in general IMO.

All of these options kind of suck.

Is there anyway to get ts-rs to encode the real number type as a comment or get the /* integer */ comment to be applied more accurately?

@TheBrokenRail TheBrokenRail changed the title /* inetger */ comments seemingly aren't applied accurately /* integer */ comments seemingly aren't applied accurately Sep 26, 2023
@MV-GH
Copy link
Contributor

MV-GH commented Sep 28, 2023

Hmm maybe someone else should try regenerating the types. Maybe something went wrong with mine. I couldn't get the DB running for my lemmy local build. But it did seem to build the databindings. On which that pr was based.

Either way the point is that it works like 2. And the script should reflect that. The only thing I can imagine is that either didnt generate all the new types or ts-rs makes numbers for things that should be originally bigints

@MV-GH
Copy link
Contributor

MV-GH commented Oct 7, 2023

@dessalines when you have some time, could you try regenerating the types. It's that the Backwards compatible API that I am working on is also a consumer of the lemmy spec

@dessalines
Copy link
Member

I tried regenerating, but it didn't change any of those ID columns.

See this line ? Its only changing the BigInts, not the Id column types, IE these

@MV-GH
Copy link
Contributor

MV-GH commented Oct 9, 2023

Yes but shouldn't ts-rs also have made bigints for those Id types? Or does it set it correctly for those as number? If that is the case, I will have to think on how to remedy that.

@dessalines
Copy link
Member

EDIT: Ignore this below, I realized there's nothing wrong with doing the same script for number


I'm looking into it in the back end, and this is what we're seeing: The ID columns are using postgres integer, which diesel calls Int4, and the other columns are using bigint (Int8).

Postgres Diesel JS
integer / serial Int4 number
bigint Int8 BigInt

So we could handle this two ways: we could either convert all the id columns in postgres from Serial -> BigSerial, or you could add lines to the script copy_generated_types... to add those integer comments to the ID columns too, which are already number.

@phiresky thoughts on converting to BigSerial?

@dessalines
Copy link
Member

dessalines commented Oct 9, 2023

@MV-GH I changed my mind on the above solution. javascript just doesn't have a way to differentiate integers from numbers, so the few floats that we added hot_rank, hot_rank_active (ts-rs also correctly converts these to numbers), really need to be handled outside.

In fact I think its a better solution to remove all of integer comments from here completely, and handle it in your importing library.

@MV-GH
Copy link
Contributor

MV-GH commented Oct 9, 2023

The problem is the currently the lemmy openapi spec is generated from the TS types. If the TS types won't have the extra type information. I will have to get it from Rust types but that increases the complexity massively. Maybe we should keep manual list of Floats. Rn this is still doable as there is only two floats. This can be done directly here, (in the conversion script) also in the form of a comment. Or I will do it on my lemmy_spec generator as that's much simpler than using Lemmy as the source. (If float problem is here not solved then we also will experience it in Jerboa)

Not that you should change lemmy for this but at some point you will probably have to increase the size of the IDs (or at least only for Comments and Posts), currently it think it is capped at 2B which quite a lot but if Lemmy keeps growing its definitely gonna hit that

@MV-GH
Copy link
Contributor

MV-GH commented Oct 20, 2023

Well this problem can be postponed for a while since all floats are removed we can simply again assume they are all integers

@dessalines
Copy link
Member

Rn this is still doable as there is only two floats

That seems like the best way to go, rather than adding all these manually.

Of course the best way to go would be to try to integrate https://github.com/juhaku/utoipa into the lemmy back-end, which we don't have the time to do rn. Nutomic tried a while back, but it had some issues, but it may be okay now with some of our recent refacvors.

@MV-GH
Copy link
Contributor

MV-GH commented Apr 28, 2024

Another approach that I came in mind is type aliases. Have a type alias for Float, Int, Long all map to number. And I would have the type info I need to differentiate between them. You would probably have to do this in Rust.

@dessalines
Copy link
Member

The ID columns do generally have type aliases: IE src/types/CommentId.ts, but counts seem to just get serialized as bigint (which we convert to number).

I looked through the API results, and couldn't find any cases of floats or doubles returned by the API, so why are these annotations necessary? Can't you just assume every number is an integer when building API docs?

@MV-GH
Copy link
Contributor

MV-GH commented Apr 29, 2024

Yes currently they are not needed for floats. But right now, I do have problems with Ints vs Longs. I am just thinking for a longterm solution because as some point we will have floats.

The ID columns do generally have type aliases: IE src/types/CommentId.ts, but counts seem to just get serialized as bigint (which we convert to number).

It won't be a clean solution but what I meant more was. Like CommentId and the likes point to another TypeAlias called for example LongType which then will be serialized as bigint. Which you can change to number but then I know all type with base aliastype LongType is a Long.

I should probably look into generating my DTOs from the Rust code base. There will always be type problems like this if I use a interim weak typed language as source

@MV-GH
Copy link
Contributor

MV-GH commented Apr 29, 2024

You can also remove these annotations If you haven't already.

@dessalines
Copy link
Member

Gotcha, I'll make a PR to remove those integer annotations.

But right now, I do have problems with Ints vs Longs.

You could probably assume Long and be safe. There's only a few cases where we use u16 anyway.

I should probably look into generating my DTOs from the Rust code base. There will always be type problems like this if I use a interim weak typed language as source

If the alternative is copying and pasting every lemmy struct in crates/db_schema, then I'd consider forking ts-rs and making an equivalent for kotlin / java. I haven't looked at its code, but it could probably work with only a few tweaks for the types and string output. That could be useful for a lot of people.

@MV-GH
Copy link
Contributor

MV-GH commented Apr 29, 2024

Another option I'll look into is using openapi as intermediate. Rust to OpenAPI. And then there should plenty of libs I can use to do openapi to java/kotlin

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

No branches or pull requests

3 participants