Binding DB sessions based on SQLAlchemy 1, changing how to declare Base Model classes, and other code modernization #5
Conversation
…se Model classes, and other code modernization - DB session binding based on SQLAlchemy 2, Base Model class declaration method change - Reflected select, delete code based on SQLAlchemy 2 - Changed how to declare Model class based on SQLAlchemy 2 - Fixed issue with social connection termination - Fixed import errors found in several examples - Added pyproject.toml file after removing setup.py due to the introduction of PEP 517/518 - Fixed minimum installed version to Python 3.7
|
Warning Rate Limit Exceeded@search5 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 8 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update focuses on making the codebase compatible with SQLAlchemy version 2, alongside modernizing the project setup to adhere to PEP 517 and PEP 518. It also includes various code style adjustments, such as standardizing quote usage and reordering imports, and updates the build and publish process for simplicity and efficiency. Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- Makefile (1 hunks)
- social_cherrypy/models.py (3 hunks)
- social_cherrypy/views.py (1 hunks)
Additional comments: 8
Makefile (3)
- 2-2: The
buildtarget uses the modernpython -m buildcommand, which is a good practice as it adheres to PEP 517/518, promoting the use of build backends.- 5-5: The
publishtarget correctly usestwine upload dist/*for publishing packages. This is a secure and recommended way to upload packages to PyPI, ensuring that credentials are handled safely.- 1-8: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-10]
The
cleantarget effectively removes bytecode files,__pycache__directories, and build artifacts. This is a crucial step in maintaining a clean workspace and avoiding potential issues with stale files.social_cherrypy/views.py (1)
- 25-25: The addition of the
csrfmiddlewaretokenparameter to thedisconnectfunction is a positive change, enhancing security by potentially mitigating CSRF attacks. However, ensure that all calls to this function are updated to include this new parameter, and consider validating the token within the function to effectively prevent CSRF attacks.CHANGELOG.md (1)
- 10-14: The changelog entries accurately reflect the significant updates made in this PR, including the transition to SQLAlchemy 2, build process modernization, Python version compatibility, and the fix for the
csrfmiddlewaretokenargument. It's important to ensure that these entries remain clear and concise to effectively communicate the changes to users and contributors.social_cherrypy/models.py (3)
- 4-5: The imports from
sqlalchemyand the use ofDeclarativeBase,Mapped, andmapped_columnfrom SQLAlchemy 2 are correctly implemented. These changes align with SQLAlchemy 2's practices for declaring models and mapping columns, promoting better compatibility and code clarity.- 15-16: The declaration of
SocialBaseas a subclass ofDeclarativeBaseis a good practice for defining the base class for all models. This ensures a consistent base class across the application, facilitating easier maintenance and upgrades.- 31-34: The refactoring of the
UserSocialAuthmodel to useMappedandmapped_columnfor defining columns is in line with SQLAlchemy 2's recommendations. This approach enhances type safety and clarity in model definitions. Ensure that the relationship betweenUserSocialAuthand theUsermodel is correctly configured, especially thebackreftosocial_auth, to maintain the integrity of the relationship and ease navigation between associated records.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (2)
- .pre-commit-config.yaml (1 hunks)
- social_cherrypy/models.py (3 hunks)
Additional comments: 2
social_cherrypy/models.py (2)
- 15-16: The introduction of the
SocialBaseclass as a custom base class for ORM models is a good practice for SQLAlchemy 2. It allows for centralized configuration and customization of model behavior. Ensure that any necessary configurations specific to your project are included in this class.- 32-35: The
UserSocialAuthmodel correctly uses SQLAlchemy 2'sMappedandmapped_columnfor declaring column types and relationships. This aligns with modern SQLAlchemy practices. However, ensure that theUID_LENGTHandUser.idare appropriately configured and exist. It's also important to verify that therelationshipwithUseris correctly set up, especially thebackrefto'social_auth', to ensure it does not introduce any unintended behavior in the user model.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .travis.yml (1 hunks)
- social_cherrypy/init.py (1 hunks)
- social_cherrypy/models.py (2 hunks)
- social_cherrypy/strategy.py (4 hunks)
- social_cherrypy/utils.py (3 hunks)
- social_cherrypy/views.py (1 hunks)
Files skipped from review due to trivial changes (1)
- social_cherrypy/init.py
Files skipped from review as they are similar to previous changes (1)
- social_cherrypy/models.py
Additional comments: 11
.travis.yml (5)
- 5-6: The environment variables
REQUIREMENTSandTEST_REQUIREMENTSare correctly set. Ensure that the paths specified in these variables are accurate and the files exist in the repository.- 11-21: The inclusion of PyPy and Python 3.3, 3.4 versions is noted. Given the PR's focus on modernization and Python 3.7 compatibility, verify if support for these older versions is still required. Python 3.3 and 3.4 have reached end-of-life and may not be compatible with newer libraries and standards.
- 25-26: The addition of
libxmlsec1-devandswigpackages underaddonsis appropriate for dependencies that require native extensions. Confirm these packages are necessary for the project's dependencies.- 28-30: The installation commands are standard and use best practices such as
travis_retryfor resilience against network issues. Ensure thatsetup.pyis still relevant given the move to PEP 517/518 standards mentioned in the PR objectives.- 32-32: The script command for running tests with coverage is correctly specified. Ensure that the
social/testsdirectory exists and contains the relevant tests.social_cherrypy/views.py (2)
- 3-3: The reorganization of import statements improves readability and maintains a logical order. This change aligns with Python best practices.
- 8-29: The changes in string literals from single to double quotes and the update of variable names within methods are cosmetic but contribute to consistency across the codebase. Ensure that these changes do not affect any string comparisons or dictionary key accesses that might rely on specific quote types.
social_cherrypy/strategy.py (2)
- 40-48: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-45]
The changes in dictionary key quotes from single to double quotes and the updates in method parameters and return values enhance consistency. These changes are cosmetic and should not impact functionality. However, ensure that all related code segments have been updated to maintain consistency.
- 64-64: Modifying the
is_responsefunction to check forstrtype instead ofsix.string_typesaligns with the move towards Python 3. This change is appropriate given the PR's objectives. Ensure that all response handling in the application is compatible with this change.social_cherrypy/utils.py (2)
- 6-10: The reordering of imports and the update of dictionary key quotes to double quotes improve code readability and consistency. These changes align with Python best practices.
- 30-60: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [19-55]
The modifications in string formatting to use double quotes consistently across the file contribute to a uniform coding style. Ensure that these changes do not affect any functionality, especially in cases where string interpolation or special characters are involved.
.travis.yml
Outdated
| python: | ||
| - "2.7" | ||
| - '2.7' |
There was a problem hiding this comment.
Python 2.7 is listed, which is no longer supported. Consider removing or updating this version to reflect the project's current Python compatibility, especially since the PR mentions ensuring compatibility with Python 3.7.
social_cherrypy/views.py
Outdated
| def disconnect(self, backend, association_id=None, csrfmiddlewaretoken=None): | ||
| user = getattr(cherrypy.request, "user", None) |
There was a problem hiding this comment.
The introduction of a csrfmiddlewaretoken parameter in the disconnect method potentially enhances security by preventing CSRF attacks. Ensure that this parameter is validated properly and used consistently across the application.
Consider implementing CSRF token validation if not already done to fully leverage the security benefits of this change.
social_cherrypy/strategy.py
Outdated
| import cherrypy | ||
|
|
||
| import six |
There was a problem hiding this comment.
The import of six is noted, which is a Python 2 and 3 compatibility library. Given the PR's focus on modernization and Python 3.7 compatibility, evaluate if the dependency on six is still necessary. If the codebase no longer supports Python 2, consider removing six and updating relevant code.
|
|
||
|
|
||
| def strategy(*args, **kwargs): | ||
| warnings.warn('@strategy decorator is deprecated, use @psa instead') | ||
| warnings.warn("@strategy decorator is deprecated, use @psa instead") |
There was a problem hiding this comment.
The deprecation warning for the @strategy decorator and the recommendation to use @psa instead is a good practice for guiding developers towards updated methods. Ensure that all instances of @strategy in the codebase have been replaced with @psa to avoid confusion.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (1)
- social_cherrypy/strategy.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- social_cherrypy/strategy.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- social_cherrypy/strategy.py (4 hunks)
- social_cherrypy/views.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- social_cherrypy/strategy.py
- social_cherrypy/views.py
…herrypy into feature/sqlalchemy2
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- social_cherrypy/strategy.py (4 hunks)
- social_cherrypy/views.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- social_cherrypy/strategy.py
- social_cherrypy/views.py
Proposed changes
With the introduction of PEP 517/518, the Python package build process has changed to allow the use of multiple build backends. We've improved the build process accordingly.
In addition, we improved the program to ensure that several previously developed libraries work properly with the newly updated SQLAlchemy 2 in January 2023.
Please consider this PR and look forward to the new version release. I took the time to fix the program and submit the patch because I'm planning to include it in a Korean Flask-based programming book I'm writing.
Types of changes
Please check the type of change your PR introduces:
Checklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Summary by CodeRabbit
csrfmiddlewaretokenargument inviews.py.