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: Dropping multiple types in Postgres #623

Merged
merged 4 commits into from
Apr 7, 2023

Conversation

PreetamSing
Copy link
Contributor

Bug Fixes

  • Following code in Rust omits the comma in SQL statement, which causes error from Postgres (as of sea-query version 0.28.3). This issue has been fixed.
use sea_query::{extension::postgres::Type, *};

#[derive(Iden)]
enum KycStatus {
   #[iden = "kyc_status"]
   Type,
   Pending,
   Approved,
}

#[derive(Iden)]
enum FontFamily {
   #[iden = "font_family"]
   Type,
   Aerial,
   Forte,
}

assert_eq!(
    Type::drop()
        .if_exists()
        .names([
            SeaRc::new(KycStatus::Type) as DynIden,
            SeaRc::new(FontFamily::Type) as DynIden,
        ])
        .cascade()
        .to_string(PostgresQueryBuilder),
    r#"DROP TYPE IF EXISTS "kyc_status""font_family" CASCADE"#
);

Changes

  • Add comma if multiple names are passed to TypeDropStatement.
  • Doc-test/example for dropping multiple types in single TypeDropStatement.

Other considerations

  • I'm fairly new to this code and have, sort of, reverse engineered this bug to fix it. Hence, I'm not fully aware of how my code can affect other things, though all the tests passed.
  • Please update the dependency in sea-orm crate, if you merge this.

Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@PreetamSing hello! Thank for the PR. A small suggestion)

src/backend/postgres/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@PreetamSing thank you! LGTM!

@ikrivosheev
Copy link
Member

@billy1624 @tyt2y3 can we make a small patch release with the fix?

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @PreetamSing, thanks for contributing!! What your thoughts on this? https://github.com/SeaQL/sea-query/pull/623/files#r1142999511

@billy1624
Copy link
Member

@billy1624 @tyt2y3 can we make a small patch release with the fix?

I agree, we should backport this to 0.28.x

@PreetamSing PreetamSing requested a review from billy1624 March 21, 2023 15:07
@ikrivosheev
Copy link
Member

ikrivosheev commented Mar 27, 2023

@billy1624, can you review the PR?

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 3, 2023

I agree, we should backport this to 0.28.x

Agreed.

@ikrivosheev ikrivosheev merged commit 7366588 into SeaQL:0.28.x Apr 7, 2023
@ikrivosheev
Copy link
Member

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

Successfully merging this pull request may close these issues.

4 participants