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: Add support for creating sequence in MariaDB #2324

Merged
merged 18 commits into from
Jan 27, 2025
Merged

Conversation

devgor88
Copy link
Contributor

@devgor88 devgor88 commented Dec 5, 2024

Description

Summary of the change: Add support for creating sequence in MariaDB.

Detailed description:

  • What: Variable supportsCreateSequence overrided from MysqlDialect(). Now it checks db version and return true for version greater than or equal to 10.3.

  • Why: It was necessary because existing isVersionCovers compares version as BigDecimal. Due to it 10.11 < 10.3.

  • How: There were added majorVersion and minorVersion in ExposedDatabaseMetadata. They were implemented in JdbcDatabaseMetadataImpl. isVersionCovers function implemented with two parameters major and minor versions.


Type of Change

Please mark the relevant options with an "X":

  • Bug fix
  • New feature
  • Documentation update

Updates/remove existing public API methods:

  • Is breaking change

Affected databases:

  • MariaDB
  • Mysql5
  • Mysql8
  • Oracle
  • Postgres
  • SqlServer
  • H2
  • SQLite

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs
  • Documentation for my change is up to date

Related Issues

@devgor88 devgor88 marked this pull request as draft December 5, 2024 10:08
@devgor88 devgor88 closed this Dec 5, 2024
@devgor88 devgor88 reopened this Dec 5, 2024
@devgor88 devgor88 marked this pull request as ready for review December 5, 2024 19:34
@devgor88 devgor88 closed this Dec 6, 2024
@devgor88 devgor88 reopened this Dec 6, 2024
@joc-a joc-a self-assigned this Dec 6, 2024
@devgor88
Copy link
Contributor Author

devgor88 commented Dec 6, 2024

I'll fix some tests

@devgor88 devgor88 marked this pull request as draft December 6, 2024 17:34
@devgor88 devgor88 marked this pull request as ready for review December 8, 2024 12:45
TransactionManager.current().db.isVersionCovers(SEQUENCE_MIN_MAJOR_VERSION, SEQUENCE_MIN_MINOR_VERSION)
}

// actually MariaDb supports it but jdbc driver prepares statement without RETURNING clause
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we implement this by ourselves? I looked how Oracle do this. As I understand Exposed sends sql without returning clause and jdbc driver add it after.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will look into it and we can deal with it in a separate pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I wanted to do it by myself earlier but I'm too busy

Copy link
Collaborator

@joc-a joc-a left a comment

Choose a reason for hiding this comment

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

Hi @devgor88. Thank you for your contribution! I left some comments.

@devgor88
Copy link
Contributor Author

devgor88 commented Dec 13, 2024

Hi @joc-a. Thank you for reviewing my contribution. I answered your comments.

@devgor88 devgor88 requested a review from joc-a December 21, 2024 17:54
@joc-a
Copy link
Collaborator

joc-a commented Jan 23, 2025

Hi @joc-a. Thank you for reviewing my contribution. I answered your comments.

Thank you, @devgor88. Looks good to me. Just one more thing to address before merging: You've ticked "Is breaking change" in the description. Could you please specify what is the breaking change? I don't think there is one, but let me know if you have a different opinion, and if you do, then it would be best to include a note about it in Breaking-Changes.md.

@devgor88
Copy link
Contributor Author

Thank you for approving, @joc-a! I wrote some comments that bothered me. I've ticked "Is breaking change" because Database.majorVersion and Database.minorVersion are public. I wasn't sure if I need. Should I edit some docs?

@joc-a
Copy link
Collaborator

joc-a commented Jan 24, 2025

Thank you for approving, @joc-a! I wrote some comments that bothered me. I've ticked "Is breaking change" because Database.majorVersion and Database.minorVersion are public. I wasn't sure if I need. Should I edit some docs?

@devgor88 Since you are neither modifying nor removing an existing public API method, but rather adding new ones, it is not a breaking change. So there is no need to tick "Is breaking change".

@devgor88
Copy link
Contributor Author

devgor88 commented Jan 24, 2025

@devgor88 Since you are neither modifying nor removing an existing public API method, but rather adding new ones, it is not a breaking change. So there is no need to tick "Is breaking change".

@joc-a Okay, I got it.

@joc-a
Copy link
Collaborator

joc-a commented Jan 27, 2025

@devgor88 Thank you for your contribution.

@joc-a joc-a merged commit e815785 into JetBrains:main Jan 27, 2025
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.

2 participants