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

Deref impls to time::* may cause API instability #618

Closed
5 tasks done
workingjubilee opened this issue Aug 12, 2022 · 1 comment · Fixed by #751
Closed
5 tasks done

Deref impls to time::* may cause API instability #618

workingjubilee opened this issue Aug 12, 2022 · 1 comment · Fixed by #751
Labels

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Aug 12, 2022

Exposing impl Deref<Target=another_crate::AnotherType> is an extreme compatibility hazard in the future if those types change, as it means PGX's stability becomes "married" to that other crate. It's more typical for these kinds of cross-crate exposures to be feature flagged to begin with. Hopefully a SemVer 1.0 release is in PGX's future, when the interfaces have settled a bit, and it might be nice to not immediately go to 2.0 and 3.0, et cetera, just because dependencies change.

Which they will, because dealing with time is one of those things that causes security issues left and right if you attempt to do anything but the most abstract possible thing.

The way the current approach works is somewhat convenient in some ways, but makes it possible to fail to represent otherwise-valid Postgres temporal values (as Postgres has a different range than most of the popular temporal Rust crates), and to potentially tamper with them in ways that may cause logically questionable results. Instead of exposing these types with infallible conversions, PGX should provide fallible TryFrom impls instead, using feature flags so that this crate does not automatically opt people into using those other crates.

These are all the cases of impl Deref to types we don't control:

impl Deref for Date {
    type Target = time::Date;
}

impl Deref for Time {
    type Target = time::Time;
}

impl Deref for TimeWithTimeZone {
    type Target = time::Time;
}

impl Deref for Timestamp {
    type Target = time::PrimitiveDateTime;
}

impl Deref for TimestampWithTimeZone {
    type Target = time::OffsetDateTime;
}
  • break Date -> time::Date
  • break Time -> time::Time
  • break TimeWithTimeZone -> time::Time
  • break Timestamp -> time::PrimitiveDateTime
  • break TimestampWithTimeZone -> time::OffsetDateTime
@workingjubilee workingjubilee changed the title Removing Deref impls to time::* Deref impls to time::* may cause API instability Aug 12, 2022
@workingjubilee
Copy link
Member Author

workingjubilee commented Aug 12, 2022

The current approach also offers a slight problem in that it has high potential overhead, since we're exposing another Rust crate's types and not necessarily a repr aligned with Postgres' internal types:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant