Skip to content

Set max length for string columns in SQLAlchemy models for MySQL compatibility#1040

Merged
seratch merged 2 commits intoslackapi:mainfrom
tattee:hotfix-sqlalchemy-oauth-model
Jun 16, 2021
Merged

Set max length for string columns in SQLAlchemy models for MySQL compatibility#1040
seratch merged 2 commits intoslackapi:mainfrom
tattee:hotfix-sqlalchemy-oauth-model

Conversation

@tattee
Copy link
Copy Markdown

@tattee tattee commented Jun 15, 2021

When I tried slackapi/bolt-python with MySQL server using SQLAlchemy, I encountered an error, 'InvalidRequestError: VARCHAR requires a length on dialect mysql'.
Therefore, I edited max length of the String in the wrappers of InstallationStore and OAuthStateStore for SQLAlchemy.
I don't know the appropriate values for the max length of Strings, so please set those values appropriately.

@gitwave gitwave bot added the untriaged label Jun 15, 2021
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 15, 2021

CLA assistant check
All committers have signed the CLA.

@seratch seratch added enhancement M-T: A feature request for new functionality oauth Version: 3x and removed untriaged labels Jun 15, 2021
@seratch seratch added this to the 3.8.0 milestone Jun 15, 2021
@seratch seratch self-requested a review June 15, 2021 21:59
@seratch
Copy link
Copy Markdown
Contributor

seratch commented Jun 15, 2021

Hi @tattee, thank you very much for takin the time to make this pull request. Before my code review, I have two things to ask:

  • Can you associate the email address that you use for git commits with your GitHub account?
  • And then, would you mind signing our CLA (see the above message by the CLA bot)?

Even if your changes are great, we are unable to merge them without signed CLA. I would appreciate it if you could understand this.

@seratch seratch changed the title set max length of String in oauth of sqlalchemy Set max length for string columns in SQLAlchemy models for MySQL compatibility Jun 15, 2021
Copy link
Copy Markdown
Contributor

@seratch seratch left a comment

Choose a reason for hiding this comment

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

If we optimize the size for each column, we can make some of them smaller but all of them are safe enough. LGTM.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 15, 2021

Codecov Report

Merging #1040 (e2e90b5) into main (0a3166c) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
- Coverage   84.33%   84.31%   -0.03%     
==========================================
  Files          95       95              
  Lines        8938     8938              
==========================================
- Hits         7538     7536       -2     
- Misses       1400     1402       +2     
Impacted Files Coverage Δ
...dk/oauth/installation_store/sqlalchemy/__init__.py 97.53% <ø> (ø)
slack_sdk/oauth/state_store/sqlalchemy/__init__.py 90.00% <ø> (ø)
slack_sdk/socket_mode/builtin/client.py 79.19% <0.00%> (-0.68%) ⬇️
slack_sdk/socket_mode/builtin/connection.py 35.71% <0.00%> (-0.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a3166c...e2e90b5. Read the comment docs.

@tattee tattee force-pushed the hotfix-sqlalchemy-oauth-model branch from 4049f4c to b805a3d Compare June 16, 2021 06:17
@seratch
Copy link
Copy Markdown
Contributor

seratch commented Jun 16, 2021

Thanks for working on this. Let me merge this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement M-T: A feature request for new functionality oauth Version: 3x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants