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

spin 3 postgres support #39

Merged
merged 2 commits into from
Nov 8, 2024
Merged

spin 3 postgres support #39

merged 2 commits into from
Nov 8, 2024

Conversation

michelleN
Copy link
Member

@michelleN michelleN commented Nov 6, 2024

This also generates the bindings for wasi kv and wasi config as they're now part of the spin platform world.

@michelleN michelleN changed the title spin 3 postgres support - wip spin 3 postgres support Nov 6, 2024
@michelleN michelleN marked this pull request as ready for review November 6, 2024 19:07
@michelleN michelleN force-pushed the postgres branch 2 times, most recently from 3cb70a3 to 28fd554 Compare November 6, 2024 20:17
Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
Co-authored-by: Brian Hardock <brian.hardock@fermyon.com>
@michelleN michelleN requested a review from a team November 6, 2024 20:20
Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if we discussed feature-gating the 3.0 stuff - can we just confirm that we happy to lean on dead code elimination rather than an explicit Cargo feature?

Otherwise looks good except I was puzzled by the wrapper types for decoding date-time types. We don't wrap other types - why these?

Also wondering if we need to do anything on the write side. What does it look like to insert a date-time value via a parameter?

Found 2 article(s) as follows:
article: Article {
id: 1,
title: "My Life as a Goat",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brought a smile to my morning!

src/pg3.rs Outdated

/// Native representation of the WIT postgres Date value.
#[derive(Clone, Debug, PartialEq)]
pub struct Date(pub chrono::NaiveDate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we need a wrapper struct here rather than decoding straight to chrono::NaiveDate? It feels like the user can't do anything the wrapper except call .0 on it to extract the NaiveDate: is it possible to cut out the middleman?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ole can't implement traits on foreign types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait maybe I gave poor advice here. is it can't implement foreign traits on foreign types?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you can't implement foreign traits on foreign types but you can totally implement local traits on foreign types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 yep yep yep.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we do exactly that on all the other types - stdlib types still count as foreign for orphan rule purposes.

src/pg3.rs Outdated

/// Native representation of the WIT postgres Time value.
#[derive(Clone, Debug, PartialEq)]
pub struct Time(pub chrono::NaiveTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip the wrapper again?

src/pg3.rs Outdated

/// Native representation of the WIT postgres DateTime value.
#[derive(Clone, Debug, PartialEq)]
pub struct DateTime(pub chrono::NaiveDateTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip the wrapper again again?

@michelleN
Copy link
Member Author

I don't remember if we discussed feature-gating the 3.0 stuff - can we just confirm that we happy to lean on dead code elimination rather than an explicit Cargo feature?

yep, relying on dead code elimination. ran it past @fibonacci1729. I think @dicej is on the same page (feel free to chime in if not Joel). Are there any benefits to feature gating other than it being more explicit?

@itowlson
Copy link
Contributor

itowlson commented Nov 7, 2024

Nah, the only thing I can think of is making the choice to opt into v3-only stuff more explicit, but it's not a big deal. I'm happy to stick with what you have.

@michelleN
Copy link
Member Author

Otherwise looks good except I was puzzled by the wrapper types for decoding date-time types. We don't wrap other types - why these?

you're right. they were unnecessary. updated!

@michelleN
Copy link
Member Author

Also wondering if we need to do anything on the write side. What does it look like to insert a date-time value via a parameter?

Here's an example of date: https://github.com/fermyon/spin-rust-sdk/pull/39/files#diff-57b15e5f7dd54bae6c97fe6ddf112edff16dc9eb6014d269f80d320280241f4cR94
Should I add a datetime example as well?

@itowlson
Copy link
Contributor

itowlson commented Nov 7, 2024

Sorry, by "as a parameter" I meant via execute parameters rather than by embedding literally into a SQL string.

E.g.

let title: String = figure_out_the_title();
let date: chrono::NaiveDate = figure_out_the_date();
conn.execute("INSERT INTO articletest(title, published) VALUES ($1, $2)", &[ /* what */ ]);

It's reasonably obvious for strings and ints and so on, I just want to see if it's still easy with dates and times where the WITs are complicated and (as you identified when we talked about it) not very obvious.

@michelleN michelleN force-pushed the postgres branch 2 times, most recently from 96c677c to a9a3ab5 Compare November 8, 2024 00:48
@michelleN
Copy link
Member Author

@itowlson good call. We added an example of doing the writes and more helpers in the module. Lmk if you see anything else we should add.

+ add support for writing date time types as parameter values

Signed-off-by: Michelle Dhanani <michelle@fermyon.com>
Co-authored-by: Brian Hardock <brian.hardock@fermyon.com>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thank you for turning it around so quickly, and for beefing up the example - this makes it a lot more helpful!

@fibonacci1729 fibonacci1729 merged commit 5e82d1a into fermyon:main Nov 8, 2024
3 checks passed
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.

update rust sdk to target Spin 3.0 world
3 participants