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

Type: Fix Modified check_ctor_args to pass default SRS_ID value in case of null #488

Conversation

satyamsoni2211
Copy link
Contributor

@satyamsoni2211 satyamsoni2211 commented Feb 7, 2024

Currently the column conversion from NullType to Geometry by SQLAlchemy is failing due to NULL SRS_ID from the DB. In such scenario, where a null value is received, function should be able to override it by default value of -1 rather than trying to convert NoneType to int. This fix takes into consideration failure and override NoneType by default value.

Below is the error that occurred due to missing SRS_ID.

  File "/Users/satyamsoni/Documents/alchemy/.venv/lib/python3.11/site-packages/geoalchemy2/types/__init__.py", line 127, in __init__
    geometry_type, srid = self.check_ctor_args(
                          ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/satyamsoni/Documents/alchemy/.venv/lib/python3.11/site-packages/geoalchemy2/types/__init__.py", line 182, in check_ctor_args
    srid = int(srid)
           ^^^^^^^^^
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

Post fix this seems to be working fine.

image

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • Please include: Fixes: #<issue number> in the description if it solves an existing issue
      (which must include a complete example of the issue).
    • Please include tests that fail with the main branch and pass with the provided fix.
  • A new feature implementation or update an existing feature
    • Please include: Fixes: #<issue number> in the description if it solves an existing issue
      (which must include a complete example of the feature).
    • Please include tests that cover every lines of the new/updated feature.
    • Please update the documentation to describe the new/updated feature.

Copy link
Member

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

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

Hi @satyamsoni2211
Thanks for this! It's indeed possible to have inappropriate srid value here.
Apart from my 2 suggestions, could you also add a test that fails on the master branch and succeeds with this PR please?

geoalchemy2/types/__init__.py Outdated Show resolved Hide resolved
geoalchemy2/types/__init__.py Outdated Show resolved Hide resolved
@satyamsoni2211
Copy link
Contributor Author

satyamsoni2211 commented Feb 8, 2024

@adrien-berchet test seems to pass now with this fix. I have added test_get_col_spec_no_srid test case for this scenario and test_get_col_spec_invalid_srid for invalid string srid.

 ~/Documents/alchemy ·················································································································································· alchemy Py │ at 09:15:53 ─╮
❯ pytest -v                                                                                                                                                                                      ─╯
======================================================================================= test session starts ========================================================================================
platform darwin -- Python 3.11.4, pytest-7.4.4, pluggy-1.4.0 -- /Users/satyamsoni/Documents/alchemy/.venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/satyamsoni/Documents/alchemy
plugins: asyncio-0.23.4, custom-exit-code-0.3.0, cov-4.1.0, mock-3.12.0
asyncio: mode=Mode.STRICT
collected 38 items                                                                                                                                                                                 

tests/test_geometry.py::TestGeometry::test_get_col_spec PASSED                                                                                                                               [  2%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_no_srid PASSED                                                                                                                       [  5%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_invalid_srid PASSED                                                                                                                  [  7%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_no_typmod PASSED                                                                                                                     [ 10%]
tests/test_geometry.py::TestGeometry::test_check_ctor_args_bad_srid PASSED                                                                                                                   [ 13%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_geometryzm PASSED                                                                                                                    [ 15%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_geometryz PASSED                                                                                                                     [ 18%]
tests/test_geometry.py::TestGeometry::test_get_col_spec_geometrym PASSED                                                                                                                     [ 21%]
tests/test_geometry.py::TestGeometry::test_check_ctor_args_srid_not_enforced PASSED                                                                                                          [ 23%]
tests/test_geometry.py::TestGeometry::test_check_ctor_args_use_typmod_nullable PASSED                                                                                                        [ 26%]
tests/test_geometry.py::TestGeometry::test_column_expression PASSED                                                                                                                          [ 28%]
tests/test_geometry.py::TestGeometry::test_select_bind_expression PASSED                                                                                                                     [ 31%]
tests/test_geometry.py::TestGeometry::test_insert_bind_expression PASSED                                                                                                                     [ 34%]
tests/test_geometry.py::TestGeometry::test_function_call PASSED                                                                                                                              [ 36%]
tests/test_geometry.py::TestGeometry::test_non_ST_function_call PASSED                                                                                                                       [ 39%]
tests/test_geometry.py::TestGeometry::test_subquery PASSED                                                                                                                                   [ 42%]
tests/test_geometry.py::TestGeography::test_get_col_spec PASSED                                                                                                                              [ 44%]
tests/test_geometry.py::TestGeography::test_get_col_spec_no_typmod PASSED                                                                                                                    [ 47%]
tests/test_geometry.py::TestGeography::test_column_expression PASSED                                                                                                                         [ 50%]
tests/test_geometry.py::TestGeography::test_select_bind_expression PASSED                                                                                                                    [ 52%]
tests/test_geometry.py::TestGeography::test_insert_bind_expression PASSED                                                                                                                    [ 55%]
tests/test_geometry.py::TestGeography::test_function_call PASSED                                                                                                                             [ 57%]
tests/test_geometry.py::TestGeography::test_non_ST_function_call PASSED                                                                                                                      [ 60%]
tests/test_geometry.py::TestGeography::test_subquery PASSED                                                                                                                                  [ 63%]
tests/test_geometry.py::TestPoint::test_get_col_spec PASSED                                                                                                                                  [ 65%]
tests/test_geometry.py::TestCurve::test_get_col_spec PASSED                                                                                                                                  [ 68%]
tests/test_geometry.py::TestLineString::test_get_col_spec PASSED                                                                                                                             [ 71%]
tests/test_geometry.py::TestPolygon::test_get_col_spec PASSED                                                                                                                                [ 73%]
tests/test_geometry.py::TestMultiPoint::test_get_col_spec PASSED                                                                                                                             [ 76%]
tests/test_geometry.py::TestMultiLineString::test_get_col_spec PASSED                                                                                                                        [ 78%]
tests/test_geometry.py::TestMultiPolygon::test_get_col_spec PASSED                                                                                                                           [ 81%]
tests/test_geometry.py::TestGeometryCollection::test_get_col_spec PASSED                                                                                                                     [ 84%]
tests/test_geometry.py::TestRaster::test_get_col_spec PASSED                                                                                                                                 [ 86%]
tests/test_geometry.py::TestRaster::test_column_expression PASSED                                                                                                                            [ 89%]
tests/test_geometry.py::TestRaster::test_insert_bind_expression PASSED                                                                                                                       [ 92%]
tests/test_geometry.py::TestRaster::test_function_call PASSED                                                                                                                                [ 94%]
tests/test_geometry.py::TestRaster::test_non_ST_function_call PASSED                                                                                                                         [ 97%]
tests/test_geometry.py::TestCompositeType::test_ST_Dump PASSED                                                                                                                               [100%]

======================================================================================== 38 passed in 0.11s ========================================================================================

Copy link
Member

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks!
Just a minor typo left.

geoalchemy2/types/__init__.py Outdated Show resolved Hide resolved
@adrien-berchet adrien-berchet merged commit 10a8316 into geoalchemy:master Feb 8, 2024
9 checks passed
@adrien-berchet
Copy link
Member

Thanks @satyamsoni2211 !
Do you want a new release with this fix?

@satyamsoni2211
Copy link
Contributor Author

satyamsoni2211 commented Feb 8, 2024

@adrien-berchet patch version bump would work 0.14.4 maybe. If you can release, I will incorporate in my project.

@satyamsoni2211
Copy link
Contributor Author

I would also like you to see if my library can be of use to you. Lazy Alchemy. This lazily reflects table from any schema and caches it.

@adrien-berchet
Copy link
Member

The new release is up! https://pypi.org/project/GeoAlchemy2/0.14.4/

About your project, that's interesting indeed. But I currently don't work on any project where the number of schemas is an issue (I actually didn't work on any project with a DB for a while 😅 ). Did you propose to integrate this into SQLAchemy? So it would be available to all the users.

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