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

Add u64 support #54

Merged
merged 9 commits into from
Jan 27, 2022
Merged

Add u64 support #54

merged 9 commits into from
Jan 27, 2022

Conversation

adzialocha
Copy link
Member

@adzialocha adzialocha commented Jan 2, 2022

  • Since p2panda-rs does not implement sqlx FromRow anymore we need to find another way to deal with p2panda structs in the database. The solution we choose here is to use std data types when interacting with the database (mostly String) and convert to specific p2panda types when leaving the database methods or vice-versa
  • Return seq_num and log_id for all JSON responses as strings to support large integers
  • Store all large numbers as strings in the database for full support of large numbers including SQLite. This might have some performance drawback, on the other hand we barely do any math with SQL with these values
  • Forcefully update past migrations with the new table structure since now the database is broken anyway (and not sure if writing a migration automatically deleting a whole table is dangerous?)
  • Unrelated but still inside of it: Make SQL varchars smaller for hashes as they also became shorter with the yasmf update

Closes: #53 and #51

Depends on: p2panda/p2panda#177

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header
  • Revert p2panda-rs to main branch in Cargo.toml

@adzialocha adzialocha linked an issue Jan 2, 2022 that may be closed by this pull request
@adzialocha adzialocha marked this pull request as ready for review January 20, 2022 11:27
@adzialocha adzialocha requested review from sandreae and cafca and removed request for sandreae January 20, 2022 11:27
@cafca
Copy link
Member

cafca commented Jan 26, 2022

I saw you removed the "breaking changes" in the changelog. Don't like it anymore? Or is it because we are pre-v1 anyway?

@adzialocha
Copy link
Member Author

I saw you removed the "breaking changes" in the changelog. Don't like it anymore? Or is it because we are pre-v1 anyway?

Haha, yes! Everything became a breaking change at one point :-D

@cafca
Copy link
Member

cafca commented Jan 26, 2022

@adzialocha have you considered testing behavior with actually corrupt u64 string representations in the db? do you think that's too unlikely to happen to have to be tested?

@cafca
Copy link
Member

cafca commented Jan 26, 2022

not sure if writing a migration automatically deleting a whole table is dangerous

I think that's fine!

@adzialocha
Copy link
Member Author

adzialocha commented Jan 26, 2022

@adzialocha have you considered testing behavior with actually corrupt u64 string representations in the db? do you think that's too unlikely to happen to have to be tested?

If this happens we might have a serious problem and the database might need to be cleaned manually (the aquadoggo will crash) or we have a bug in our validation logic or somewhat corrupt the data while processing it in our code. I think that these problems should be migitated by making sure they don't happen before they get written into the database (aka test our validation logic well), writing a test which contains invalid data in the database will only test that it will crash, and this might be well .. "too late" 😿 😄

@adzialocha
Copy link
Member Author

adzialocha commented Jan 26, 2022

not sure if writing a migration automatically deleting a whole table is dangerous

I think that's fine!

Not sure about this. If I want to delete my database I want to do it manually .. imagine aquadoggo becomes 2 years old one day and for whatever reason someone does something with the migrations and all data is "automatically" gone 😢

@cafca
Copy link
Member

cafca commented Jan 26, 2022

writing a test which contains invalid data in the database will only test that it will crash

In the ideal case, yes. We will only show that error when a db is beyond repair. If we show that error and the db is okay it would be really confusing, that's what I was thinking about.

@cafca
Copy link
Member

cafca commented Jan 26, 2022

not sure if writing a migration automatically deleting a whole table is dangerous

I think that's fine!

Not sure about this. If I want to delete my database I want to do it manually .. imagine aquadoggo becomes 2 years old one day and for whatever reason someone does something with the migrations and all data is "automatically" gone 😢

I don't understand, why would we do that? Deleting a database is different from deleting a table.

@adzialocha
Copy link
Member Author

adzialocha commented Jan 26, 2022

Deleting a database is different from deleting a table.

Yes, of course. Still, deleting a table automatically without any warning is still dangerous. I don't expect migrations to delete data for me, I'd rather let the migration fail and notify the user that I need to manually delete a database or table or any sort of data.

"Deleting" data is very different from migrating it from one format to another, which might include deleting tables. But then I didn't loose data ..

In the ideal case, yes. We will only show that error when a db is beyond repair. If we show that error and the db is okay it would be really confusing, that's what I was thinking about.

We can add a test if you want to check if its crashing 👍

@cafca
Copy link
Member

cafca commented Jan 26, 2022

Hm, ok, then we disagree there. I think we will have lots of tables being created and deleted automatically anyway and don't see a problem with well thought through migrations also doing that.

@adzialocha
Copy link
Member Author

adzialocha commented Jan 26, 2022

Hm, ok, then we disagree there. I think we will have lots of tables being created and deleted automatically anyway and don't see a problem with well thought through migrations also doing that.

If the deleted table gets converted into a new table I don't mind. If it actually contains data I wanted to keep but gets removed forever, then I don't agree.

@cafca
Copy link
Member

cafca commented Jan 26, 2022

We can add a test if you want to check if its crashing 👍

I think it's fine as it is. I just wanted to think it through a bit :)

@cafca
Copy link
Member

cafca commented Jan 26, 2022

If it actually contains data I wanted to keep but gets removed forever, then I don't agree.

Actually reminds be of another topic I would love to get back to: ephemeral data.

@cafca
Copy link
Member

cafca commented Jan 26, 2022

I would like to add a section on the whole u64 issue to the handbook. Question: Where exactly is it not supported? I know about JSON and JavaScript. For SQLite I find this page, which says it's supported, but the issue description also says it's not supported there. I don't quite remember - what was the issue with SQLite?

@adzialocha
Copy link
Member Author

adzialocha commented Jan 26, 2022

I would like to add a section on the whole u64 issue to the handbook. Question: Where exactly is it not supported? I know about JSON and JavaScript. For SQLite I find this page, which says it's supported, but the issue description also says it's not supported there. I don't quite remember - what was the issue with SQLite?

Think this summarises it quite nicely: launchbadge/sqlx#1374

@cafca cafca merged commit ad4fb7d into main Jan 27, 2022
@cafca cafca deleted the u64-support branch January 27, 2022 16:00
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.

Store u64 as strings Reflect new yasmf hash sizes in SQL tables
2 participants