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] Attribute error when trying to get bounding box from a method field value #246

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hemache
Copy link

@hemache hemache commented Nov 5, 2020

fixes #247

@TravisBuddy
Copy link

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: f72e69c0-1f9e-11eb-bd2e-17a8c440c3de

@TravisBuddy
Copy link

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 84aa81b0-1fc4-11eb-9dab-419bf2960c30

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @hemache, see my question below.

if isinstance(field, SerializerMethodField):
method = getattr(field.parent, field.method_name)
geo_value = method(instance)
feature["geometry"] = field.to_representation(instance)
Copy link
Member

Choose a reason for hiding this comment

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

is using instance instead of geo_value intended?

Copy link
Author

Choose a reason for hiding this comment

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

is using instance instead of geo_value intended?

yes, because in this case, SerializerMethodField.to_representation receives object instance instead of the actual geometry value.

the problem was that geo_value initially stores the object instance (which doesn't provide .extent attribute), so here we're making sure to actually get the geometry value instead of the object instance.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I don't understand then what's the purpose of calling geo_value = method(instance), it seems like that line can be removed, since geo_value is not used afterwards. Isn't it?
But I am also not entirely convinced of the approach as explained above.

Copy link
Author

Choose a reason for hiding this comment

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

since geo_value is not used afterwards. Isn't it?

yes, it's not used afterwards, but what if -later- someone else needed to use it?
I believe geo_value should have the correct/actual GEO value, not an instance of the object.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this. I don't want to introduce changes in a library that is used by thousands of developers lightly. Each change must be justified.

Copy link
Author

Choose a reason for hiding this comment

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

@nemesisdesign it's justified. did you try to reproduce this bug?

focus on the type of geo_value variable. it should hold a GEO value and not something else, right?

Copy link
Author

Choose a reason for hiding this comment

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

@nemesisdesign if you're not convinced by my solution, then please try to reproduce the bug #247 and maybe suggest a proper fix?

@TravisBuddy
Copy link

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: c2b92120-1fe0-11eb-9dab-419bf2960c30

@hemache
Copy link
Author

hemache commented Nov 6, 2020

@nemesisdesign any idea why the CI fails because it tries to access PostgreSQL?

@atb00ker
Copy link
Member

atb00ker commented Nov 8, 2020

@nemesisdesign any idea why the CI fails because it tries to access PostgreSQL?

@hemache Could you please try to change to dist: bionic in .travis.yml? It might help!

@TravisBuddy
Copy link

Hey @hemache,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: e2c65be0-21c0-11eb-a04c-b76cbd6ddda3

@atb00ker
Copy link
Member

atb00ker commented Nov 8, 2020

Well, that didn't exactly help.

On inspection, I see:

Ver Cluster Port Status Owner    Data directory              Log file

12  main    5433 down   postgres /var/lib/postgresql/12/main /var/log/postgresql/postgresql-12-main.log

Perhaps cat /var/lib/postgresql/12/main /var/log/postgresql/postgresql-12-main.log would help. It should be a postgresql version problem, changing and experimenting with some other versions could help.

@hemache hemache force-pushed the fix-geoserializer-bbox-getter branch 3 times, most recently from b03cad0 to a52ae4b Compare November 11, 2020 18:45
@hemache hemache force-pushed the fix-geoserializer-bbox-getter branch from ebcba18 to 0040739 Compare November 11, 2020 19:09
@hemache
Copy link
Author

hemache commented Nov 11, 2020

fixed PostgreSQL issue.

but now I'm getting this error (e.g. https://travis-ci.com/github/openwisp/django-rest-framework-gis/jobs/433971220)

free(): invalid pointer
ERROR: InvocationError for command /home/travis/build/openwisp/django-rest-framework-gis/.tox/travis/bin/coverage run --source=rest_framework_gis ./tests/manage.py test tests/django_restframework_gis_tests (exited with code -6 (SIGABRT)) (exited with code -6)
___________________________________ summary ____________________________________
ERROR:   travis: commands failed
The command "tox -e travis" exited with 1.

though, I can't reproduce this in my local dev env

coverage run --source=rest_framework_gis ./tests/manage.py test tests/django_restframework_gis_tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
............................s.........
GeoJSON rendering of 10000 objects completed in 6.299168238998391
.....s......./home/hemache/.local/share/virtualenvs/django-rest-framework-gis-EHv3zJno/lib/python3.8/site-packages/djangorestframework-3.12.1-py3.8.egg/rest_framework/pagination.py:200: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: <class 'django_restframework_gis_tests.models.Location'> QuerySet.
  paginator = self.django_paginator_class(queryset, page_size)
............................
----------------------------------------------------------------------
Ran 79 tests in 63.766s

OK (skipped=2)
Destroying test database for alias 'default'...

any ideas?

@nemesifier
Copy link
Member

@hemache did you already try using bionic as distro? Sometimes changing distro fixes these inexplicable issues.

@hemache
Copy link
Author

hemache commented Nov 12, 2020

@nemesisdesign yep, I tried that before

The following packages have unmet dependencies:
 gdal-bin : Depends: python3-gdal (= 3.0.4+dfsg-1~bionic0) but it is not going to be installed
            Depends: libgdal26 (>= 3.0.0) but it is not going to be installed
E: Unable to correct problems, you have held broken packages.
The command "sudo apt-get install binutils libproj-dev gdal-bin -y" failed and exited with 100 during .

https://travis-ci.com/github/openwisp/django-rest-framework-gis/jobs/434284716

can we please merge this anyway and fix CI issues later? (tests are passing btw)

@hemache
Copy link
Author

hemache commented Dec 1, 2020

@nemesisdesign any update on this, please? we're still using the patched version from my forked Github repo, I hope we can switch back to the official version in PyPi

Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

Just one comment from my end,
I'll defer the rest to @nemesisdesign

.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I am not sure of this change.
There's some repetition to check if the field is a SerializerMethodField.
I guess it could be resolved by changing the way SerializerMethodField and GeoFeatureModelSerializer interact.

I will look into it as soon as possible.

if isinstance(field, SerializerMethodField):
method = getattr(field.parent, field.method_name)
geo_value = method(instance)
feature["geometry"] = field.to_representation(instance)
Copy link
Member

Choose a reason for hiding this comment

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

ok, I don't understand then what's the purpose of calling geo_value = method(instance), it seems like that line can be removed, since geo_value is not used afterwards. Isn't it?
But I am also not entirely convinced of the approach as explained above.

@hemache
Copy link
Author

hemache commented Dec 8, 2020

do you think this is ready to be merged?

@auvipy
Copy link
Collaborator

auvipy commented Dec 9, 2020

this need rebase

@nemesifier
Copy link
Member

Not ready to merge: #246 (comment)

@nemesifier nemesifier force-pushed the master branch 2 times, most recently from d137601 to f2dc812 Compare May 9, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

AttributeError: object has no attribute 'extent'
5 participants