[fix] Attribute error when trying to get bounding box from a method field value #246

hemache · 2020-11-05T16:18:51Z

fixes #247

…ield value

TravisBuddy · 2020-11-05T19:41:59Z

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 · 2020-11-06T00:10:48Z

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

nemesifier

Thanks for contributing @hemache, see my question below.

nemesifier · 2020-11-06T02:49:02Z

rest_framework_gis/serializers.py

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


is using instance instead of geo_value intended?

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.

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.

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.

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.

@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?

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

TravisBuddy · 2020-11-06T03:32:58Z

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 · 2020-11-06T10:43:09Z

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

atb00ker · 2020-11-08T10:27:10Z

@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 · 2020-11-08T12:49:51Z

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 · 2020-11-08T13:18:59Z

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.

…er authentication for user "postgres"

hemache · 2020-11-11T20:29:49Z

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 · 2020-11-11T21:23:43Z

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

hemache · 2020-11-12T09:10:56Z

@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 · 2020-12-01T15:42:22Z

@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

atb00ker

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

.travis.yml

tests/django_restframework_gis_tests/urls.py

nemesifier

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.

nemesifier · 2020-12-02T15:59:13Z

rest_framework_gis/serializers.py

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


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 · 2020-12-08T11:15:20Z

do you think this is ready to be merged?

auvipy · 2020-12-09T05:20:09Z

this need rebase

nemesifier · 2021-01-23T15:13:15Z

Not ready to merge: #246 (comment)

hemache added 2 commits November 5, 2020 17:15

[fix] attribute error when trying to get bounding box from a method f…

58ec8d0

…ield value

[qa] Test bounding box calculation when using method field for geo field

1d57899

[qa] Format code

191209b

nemesifier reviewed Nov 6, 2020

View reviewed changes

hemache force-pushed the fix-geoserializer-bbox-getter branch 3 times, most recently from b03cad0 to a52ae4b Compare November 11, 2020 18:45

[qa] Change PostgreSQL database port back to default value & Allow pe…

0040739

…er authentication for user "postgres"

hemache force-pushed the fix-geoserializer-bbox-getter branch from ebcba18 to 0040739 Compare November 11, 2020 19:09

hemache force-pushed the fix-geoserializer-bbox-getter branch from f7b09ed to 0040739 Compare November 12, 2020 09:11

nemesifier added this to In progress in OpenWISP Priorities for next releases via automation Nov 13, 2020

nemesifier added the bug label Nov 13, 2020

nemesifier moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Nov 13, 2020

atb00ker reviewed Dec 1, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

auvipy reviewed Dec 2, 2020

View reviewed changes

tests/django_restframework_gis_tests/urls.py Show resolved Hide resolved

[qa] Rollback CI PostgreSQL default port fix

84cd642

nemesifier reviewed Dec 2, 2020

View reviewed changes

devkapilbansal moved this from Ready for review/testing to In progress in OpenWISP Priorities for next releases Mar 21, 2021

nemesifier moved this from In progress to Backlog in OpenWISP Priorities for next releases Jan 25, 2022

nemesifier force-pushed the master branch 2 times, most recently from d137601 to f2dc812 Compare May 9, 2022 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Attribute error when trying to get bounding box from a method field value #246

[fix] Attribute error when trying to get bounding box from a method field value #246

hemache commented Nov 5, 2020 •

edited

TravisBuddy commented Nov 5, 2020

TravisBuddy commented Nov 6, 2020

nemesifier left a comment

nemesifier Nov 6, 2020

hemache Nov 6, 2020

nemesifier Dec 2, 2020

hemache Dec 3, 2020

nemesifier Jan 23, 2021

hemache Feb 8, 2021

hemache Feb 8, 2021

TravisBuddy commented Nov 6, 2020

hemache commented Nov 6, 2020

atb00ker commented Nov 8, 2020 •

edited

TravisBuddy commented Nov 8, 2020

atb00ker commented Nov 8, 2020

hemache commented Nov 11, 2020

nemesifier commented Nov 11, 2020

hemache commented Nov 12, 2020

hemache commented Dec 1, 2020

atb00ker left a comment

nemesifier left a comment

nemesifier Dec 2, 2020

hemache commented Dec 8, 2020

auvipy commented Dec 9, 2020

nemesifier commented Jan 23, 2021

[fix] Attribute error when trying to get bounding box from a method field value #246

Are you sure you want to change the base?

[fix] Attribute error when trying to get bounding box from a method field value #246

Conversation

hemache commented Nov 5, 2020 • edited

TravisBuddy commented Nov 5, 2020

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

TravisBuddy commented Nov 6, 2020

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

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Nov 6, 2020

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

hemache commented Nov 6, 2020

atb00ker commented Nov 8, 2020 • edited

TravisBuddy commented Nov 8, 2020

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

atb00ker commented Nov 8, 2020

hemache commented Nov 11, 2020

nemesifier commented Nov 11, 2020

hemache commented Nov 12, 2020

hemache commented Dec 1, 2020

atb00ker left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemache commented Dec 8, 2020

auvipy commented Dec 9, 2020

nemesifier commented Jan 23, 2021

hemache commented Nov 5, 2020 •

edited

atb00ker commented Nov 8, 2020 •

edited