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

fix(postgres): PgHasArrayType is not implemented for Vec<i64> #2617

Closed
wants to merge 1 commit into from

Conversation

mrl5
Copy link
Contributor

@mrl5 mrl5 commented Jul 14, 2023

caused by #2086
closes #2611

context

seems like #2086 fixed issue described by @Wopple by making that block of code unreachable. Not as it was stated in

Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

rationale for reverting that change

after checking https://doc.rust-lang.org/std/macro.cfg.html

cfg!, unlike #[cfg], does not remove any code and only evaluates
to true or false. For example, all blocks in an if/else expression
need to be valid when cfg! is used for the condition, regardless of
what cfg! is evaluating.

so to my understanding it would be true anyway in @woople scenario

caused by launchbadge#2086
closes launchbadge#2611

seems like launchbadge#2086 fixed issue described by @Wopple by making that block
of code unreachable. Not as it was stated in
> Problem: PgHasArrayType was checking the application's postgres feature
> Solution: only check the library's postgres feature

after checking https://doc.rust-lang.org/std/macro.cfg.html
> `cfg!`, unlike `#[cfg]`, does not remove any code and only evaluates
> to true or false. For example, all blocks in an if/else expression
> need to be valid when `cfg!` is used for the condition, regardless of
> what `cfg!` is evaluating.

so to my understanding it would be `true` anyway in @woople scenario
@jplatte
Copy link
Contributor

jplatte commented Jul 14, 2023

Have you verified that this actually fixes your problem? I can't see how that would be possible. The thing you are reverting did make sense, since it does a cfg check for the sqlx-macro crate's features, instead of the features of the crate in which the derive is expanded.

@mrl5
Copy link
Contributor Author

mrl5 commented Jul 14, 2023

@jplatte I did verify that it fixes my problem:

  1. in simple repro env given in PgHasArrayType is not implemented for Vec<i64> #2611
  2. here chore(backend): upgrade sqlx to ^0.7 windmill-labs/windmill#1865 using my patch

if you have another idea how this can be fixed/workarounded then please feel free to reproduce based on steps from #2611

@jplatte
Copy link
Contributor

jplatte commented Jul 14, 2023

Oh, I think I get it. You don't actually intend to ever have arrays of ScriptHashes, presumably?

@mrl5
Copy link
Contributor Author

mrl5 commented Jul 14, 2023

ScriptHashes are used in windmill codebase (e.g. here)

Steps from #2611 are just minimalistic and isolated environment for reproducing the bug introduced in #2086

@jplatte
Copy link
Contributor

jplatte commented Jul 14, 2023

So, the problem is that the impl PgHasArrayType would make sense to implement automatically in the sqlx::Type derive when it's possible to implement, but the macro can't know that. Currently it's just implemented unconditionally, which is useful when it works, but problematic when it doesn't like in your case.

The idiomatic thing would probably be to never generate the impl from the Type derive and introduce a separate derive macro for the array trait. But that would be a breaking change, and I'm sure people are already relying on the Type macro generating this impl. I think the best solution for sqlx 0.7 is to allow people to opt out of the array trait impl.

@abonander
Copy link
Collaborator

Yeah, @jplatte's assessment is correct. It appears a non-generic impl will throw an error if its where bounds are not satisfied, which is what the generated impl relies on: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4a5de30ae88649518f63648226140e89

Theoretically, Vec<i64> should be able to implement PgHasArrayType as that would allow us to define multidimensional arrays, but the driver doesn't current support encoding or decoding those, which gets a little weird. I'm not sure it would be possible to ensure the dimensionality is correct at compile time, as that's actually a dynamic property of arrays in Postgres' type system, so we could only provide very minimal guarantees about them. It was just easier to punt on the problem. I think we would want a special array type that supports dynamic dimensionality, which sounds like a fun experiment but I just haven't had time to dig into it.

Unfortunately, just straight reverting this change will break other cases where the derive is successfully compiled and being used, so I'm unable to accept this PR as-is.

However, I'm going to get working on a fix straight away, as I do agree this is a problem. That's most likely just going to involve an attribute that allows one to opt out of the array derive.

For 0.8 I may consider separating PgHasArrayType out into its own derive, but I think this is more of an edge case, honestly, considering no one else has reported something similar yet. including during the almost six months that 0.7 was in alpha.

@abonander abonander closed this Jul 14, 2023
@Wopple
Copy link
Contributor

Wopple commented Jul 15, 2023

@mrl5

Complete guess here, but would something like this help?

#[derive(sqlx::Type)]
#[sqlx(transparent)]
pub struct ScriptHash(pub i64);

pub type ScriptHashes = Vec<ScriptHash>;

@mrl5
Copy link
Contributor Author

mrl5 commented Jul 16, 2023

hello @Wopple, thanks for reaching out. Your proposal indeed compiles for

Steps from #2611 are just minimalistic and isolated environment for reproducing (...)

I've tried it with slight modification running sqlx 0.7.0 in actual windmill codebase

 pub struct ScriptHash(pub i64);
 
  #[derive(PartialEq)]
 #[cfg_attr(feature = "sqlx", derive(sqlx::Type))]
-#[cfg_attr(feature = "sqlx", sqlx(transparent))]
 pub struct ScriptHashes(pub Vec<i64>);
 
 impl Display for ScriptHash {
@@ -91,7 +90,7 @@ impl Serialize for ScriptHashes {
     {
         let mut seq = serializer.serialize_seq(Some(self.0.len()))?;
         for element in &self.0 {
-            seq.serialize_element(&ScriptHash(*element))?;
+            seq.serialize_element(element)?;
         }
         seq.end()
     }

and indeed it compiles now 🙇 The best part is that to my recollection I experimented with it, before bothering ppl here.

Not sure why I abandoned this approach. Perhaps I hit errors caused by unrelated breaking change (linked below), that - in frustration - I assumed to be related ...

// In 0.7, `Transaction` can no longer implement `Executor` directly,
// so it must be dereferenced to the internal connection type.
.execute(&mut **transaction)
.await?;

===
anyways the bottom line is that thanks to Jonas and Austin fix available in sqlx 0.7.1 works for both my scenario and your scenario. No regression on my end and I hope that there is also no regression on your end @Wopple

Looks like above patch would work with 0.7.0 but I must admit that I didn't test it - I only checked if code compiles

@mrl5
Copy link
Contributor Author

mrl5 commented Jul 16, 2023

update

actually workaround suggested by @Wopple fails at runtime, so #2619 is the only known fix at this moment for us

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.

PgHasArrayType is not implemented for Vec<i64>
4 participants