Skip to content

[IMP] 19.0.local messy #6

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.

Sign up for GitHub

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

Merged
weinni2000 merged 2 commits into from
Jan 28, 2026
Merged

[IMP] 19.0.local messy #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf1990
without odoo-test-helper
weinni2000 Oct 29, 2025
3f8eefe
Co-authored-by: Nik weinni2000@gmail.com
weinni2000 Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions base_geoengine/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ The module also requires two additional python libs:
- `geojson <http://pypi.python.org/pypi/geojson>`__

When you will install the module this two additional libs will be
installed.

For a complete documentation please refer to the `public
installed. For a complete documentation please refer to the `public
documenation <http://oca.github.io/geospatial/index.html>`__
Comment on lines +79 to 80
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

issue (typo): Fix typo "documenation" and consider improving the article before "complete documentation".

'documentation' is misspelled, and "the complete documentation" or just "complete documentation" would be more natural here.

Suggested change
installed. For a complete documentation please refer to the `public
documenation <http://oca.github.io/geospatial/index.html>`__
installed. For complete documentation please refer to the `public
documentation <http://oca.github.io/geospatial/index.html>`__


Usage
Expand Down
Binary file modified base_geoengine/__pycache__/__init__.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/__pycache__/domains.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/__pycache__/fields.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/__pycache__/geo_convertion_helper.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/__pycache__/geo_db.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
12 changes: 6 additions & 6 deletions base_geoengine/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import string
from operator import attrgetter

from odoo import fields
from odoo import _, fields
from odoo.fields import Domain
from odoo.models import BaseModel
from odoo.orm.fields import Field
Expand Down Expand Up @@ -281,7 +281,7 @@ def entry_to_shape(self, value, same_type=False):
shape = convert.value_to_shape(value)
if same_type and not shape.is_empty:
if shape.geom_type.lower() != self.geo_type.lower():
msg = self.env._(
msg = _(
"Geo Value %(geom_type)s must be of the same type %(geo_type)s \
as fields",
geom_type=shape.geom_type.lower(),
Expand All @@ -301,14 +301,14 @@ def update_geo_db_column(self, model):
check_data = cr.fetchone()
if not check_data:
raise TypeError(
self.env._(
_(
"geometry_columns table seems to be corrupted."
" SRID check is not possible"
)
)
if check_data[0] != self.srid:
raise TypeError(
self.env._(
_(
"Reprojection of column is not implemented."
" We can not change srid %(srid)s to %(data)s",
srid=self.srid,
Expand All @@ -317,7 +317,7 @@ def update_geo_db_column(self, model):
)
elif check_data[1] != self.geo_type.upper():
raise TypeError(
self.env._(
_(
"Geo type modification is not implemented."
" We can not change type %(data)s to %(geo_type)s",
data=check_data[1],
Expand All @@ -326,7 +326,7 @@ def update_geo_db_column(self, model):
)
elif check_data[2] != self.dim:
raise TypeError(
self.env._(
_(
"Geo dimention modification is not implemented."
" We can not change dimention %(data)s to %(dim)s",
data=check_data[2],
Expand Down
49 changes: 35 additions & 14 deletions base_geoengine/geo_convertion_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,27 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
import logging

from shapely.errors import GEOSException

from odoo import _

logger = logging.getLogger(__name__)
_logger = logging.getLogger(__name__)

try:
import geojson
from shapely import wkb, wkt
from shapely.geometry import shape
from shapely.geometry.base import BaseGeometry
except ImportError:
logger = logging.getLogger(__name__)
logger.warning(_("Shapely or geojson are not available in the sys path")) # pylint: disable=prefer-env-translation
_logger.warning(_("Shapely or geojson are not available in the sys path")) # pylint: disable=prefer-env-translation


def value_to_shape(value, use_wkb=False):
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

issue (complexity): Consider simplifying value_to_shape by clarifying the use_wkb parameter semantics, extracting WKB/WKT parsing into helpers, and removing noisy inline comments to make the control flow easier to follow.

You can keep the new behavior while reducing complexity and making the flow easier to reason about.

1. Make use_wkb semantics explicit instead of overriding

Right now the parameter is misleading:

def value_to_shape(value, use_wkb=False):
    """Transforms input into a Shapely object"""
    # always use_wkb for now
    use_wkb = True

This makes the signature lie. To keep behavior but reduce cognitive load:

  • Keep the parameter for API compatibility.
  • Explicitly ignore it and document that it’s currently forced to WKB.
def value_to_shape(value, use_wkb=False):
    """Transforms input into a Shapely object.

    NOTE: `use_wkb` is currently ignored and WKB is always tried first
    for non-JSON strings. The parameter is kept for backward compatibility.
    """
    # keep `use_wkb` in signature but avoid misleading overrides
    _ = use_wkb  # intentionally unused

    if not value:
        return wkt.loads("GEOMETRYCOLLECTION EMPTY")
    ...

This removes the hidden override while preserving the “always WKB first” behavior introduced by your change.

2. Extract WKB/WKT parsing into small helpers

You can linearize the control flow and unify error handling/logging by pushing the try/except into helpers:

def _loads_wkb(value: str):
    """Try to load WKB; return geometry or None on failure."""
    try:
        return wkb.loads(value, hex=True)
    except GEOSException:
        _logger.warning("GEOSException during WKB load", exc_info=True)
    except Exception:
        _logger.warning("WKB conversion failed", exc_info=True)
    return None


def _loads_wkt_with_fallback(value: str):
    """Load WKT, falling back to POINT(0 0) on failure."""
    try:
        return wkt.loads(value)
    except Exception as e:
        _logger.warning("Failed to parse WKT: %s", e)
        return wkt.loads("POINT(0 0)")

Then value_to_shape becomes simpler and easier to scan:

def value_to_shape(value, use_wkb=False):
    """Transforms input into a Shapely object.

    NOTE: `use_wkb` is currently ignored and WKB is always tried first
    for non-JSON strings. The parameter is kept for backward compatibility.
    """
    _ = use_wkb  # intentionally unused

    if not value:
        return wkt.loads("GEOMETRYCOLLECTION EMPTY")

    if isinstance(value, str):
        if "{" in value:
            geo_dict = geojson.loads(value)
            return shape(geo_dict)

        # always try WKB first
        geom = _loads_wkb(value)
        if geom is not None:
            return geom

        # WKB failed → WKT with POINT(0 0) fallback
        return _loads_wkt_with_fallback(value)

    if hasattr(value, "wkt"):
        return value if isinstance(value, BaseGeometry) else wkt.loads(value.wkt)

    raise TypeError(
        _(
            "Write/create/search geo type must be wkt/geojson "
            "string or must respond to wkt"
        )
    )

This keeps all current semantics:

  • WKB always attempted for non-JSON strings.
  • On WKB failure, falls back to WKT.
  • On WKT failure, returns POINT(0 0).

3. Remove or relocate the large commented-out block

The long inline commented block about hash values and old behavior adds noise in the middle of the core path:

# elif len(value) == 42 and "POINT" not in value and "(" not in value:
# not clear why use_wkb is not set correctly anymore.
# ...
#        value = wkb.loads(value, hex=True)
#        return value

If you want to preserve this rationale, move it out of the main function body, e.g.:

# NOTE: Historical behavior:
# - There used to be a special case for 42-character strings without "POINT" or "("
#   to handle hash-like WKB values such as
#   "0101000000A4703D0AD7232C400AD7A3703D0A4840".
# - This is currently not supported; logic is kept here for reference:
#   (see git history or design docs).

or into the module docstring/design notes. That keeps value_to_shape focused and easier to read.

"""Transforms input into a Shapely object"""

# always use_wkb for now
use_wkb = True
Comment on lines +23 to +24
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

issue: Overriding use_wkb unconditionally makes the function parameter misleading and may mask configuration issues.

Forcing use_wkb = True here makes the argument meaningless and ignores caller intent. If this is intentional but temporary, either remove the parameter and update callers, or hide this behind a dedicated configuration flag. At minimum, add a clear TODO explaining why this override exists and under what conditions it should be reverted to avoid this silent behavior change becoming permanent.


if not value:
return wkt.loads("GEOMETRYCOLLECTION EMPTY")
if isinstance(value, str):
Expand All @@ -26,19 +31,35 @@ def value_to_shape(value, use_wkb=False):
if "{" in value:
geo_dict = geojson.loads(value)
return shape(geo_dict)
elif use_wkb:
return wkb.loads(value, hex=True)
else:
# <NIKMOD>
# 'POINT(0.0 0.0)'

if use_wkb:
try:
return wkt.loads(value)
res = wkb.loads(value, hex=True)
return res
except GEOSException as e:
_logger.warning("GEOSException:")
_logger.warning(e)
Comment on lines +38 to +40
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

suggestion: Exception logging for GEOS/WKB failures could include more context to aid debugging.

Right now the GEOSException handler logs only a label and the exception. Please log some minimal context about the input (e.g. type and/or length) and pass exc_info=True so we get the traceback, for example:

_logger.warning(
    "GEOSException while loading WKB (len=%s)",
    len(value),
    exc_info=True,
)

The same improvement would help in the generic WKB conversion failure case as well.

except Exception as e:
logger.warning(_("Failed to parse WKT: %s", e)) # pylint: disable=prefer-env-translation
empty = "POINT(0.0 0.0)"
return wkt.loads(empty)
# </NIKMOD>
_logger.warning("WKB conversion failed:")
_logger.warning(e)
# elif len(value) == 42 and "POINT" not in value and "(" not in value:
# not clear why use_wkb is not set correctly anymore.
# if a record is stored it's instantly
# executed again with the hash value
# The hash value eg "0101000000A4703D0AD7232C400AD7A3703D0A4840"
# is also possible, but we do not handle it now.
# value = wkb.loads(value, hex=True)
# return value
# <NIKMOD>
# 'POINT(0.0 0.0)'
try:
return wkt.loads(value)
except Exception as e:
# error = _("Failed to parse WKT: %(e)s") % {"e":e}
error = f"Failed to parse WKT: {e}"
_logger.warning(error) # pylint: disable=prefer-env-translation
empty = "POINT(0 0)"
return wkt.loads(empty)
# </NIKMOD>
elif hasattr(value, "wkt"):
return value if isinstance(value, BaseGeometry) else wkt.loads(value.wkt)
else:
Expand Down
Binary file modified base_geoengine/models/__pycache__/__init__.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/models/__pycache__/base.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/models/__pycache__/geo_raster_layer.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/models/__pycache__/geo_vector_layer.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/models/__pycache__/ir_model.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file modified base_geoengine/models/__pycache__/ir_view.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
1 change: 0 additions & 1 deletion base_geoengine/readme/INSTALL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,5 @@ The module also requires two additional python libs:

When you will install the module this two additional libs will be
installed.

For a complete documentation please refer to the [public
documenation](http://oca.github.io/geospatial/index.html)
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

issue (typo): Correct the typo "documenation" and consider adjusting "a complete documentation".

This nearby line has the same typo: "documenation" → "documentation". Also consider changing "a complete documentation" to "the complete documentation" or simply "complete documentation" for better grammar.

3 changes: 1 addition & 2 deletions base_geoengine/static/description/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,7 @@ <h2><a class="toc-backref" href="#toc-entry-1">Installation</a></h2>
<li><a class="reference external" href="http://pypi.python.org/pypi/geojson">geojson</a></li>
</ul>
<p>When you will install the module this two additional libs will be
installed.</p>
<p>For a complete documentation please refer to the <a class="reference external" href="http://oca.github.io/geospatial/index.html">public
installed. For a complete documentation please refer to the <a class="reference external" href="http://oca.github.io/geospatial/index.html">public
documenation</a></p>
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

nitpick (typo): Minor spelling issue in the updated sentence.

The link text currently says public documenation; please correct this to public documentation.

Suggested change
documenation</a></p>
documentation</a></p>

</div>
<div class="section" id="usage">
Expand Down
Binary file modified base_geoengine/tests/__pycache__/__init__.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file removed base_geoengine/tests/__pycache__/models.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file removed base_geoengine/tests/__pycache__/test_model.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
Binary file removed base_geoengine/tests/__pycache__/test_nikstests.cpython-310.pyc
View file
Open in desktop
Binary file not shown.
30 changes: 24 additions & 6 deletions base_geoengine/tests/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
# Copyright 2023 ACSONE SA/NV

import geojson
from odoo_test_helper import FakeModelLoader

# from odoo_test_helper import FakeModelLoader
from shapely import wkt
from shapely.geometry import shape

from odoo.orm.model_classes import add_to_registry
from odoo.tests.common import TransactionCase

from ..fields import GeoPoint
Expand All @@ -14,12 +16,25 @@ class TestModel(TransactionCase):
@classmethod
def setUpClass(cls):
super().setUpClass()
cls.loader = FakeModelLoader(cls.env, cls.__module__)
cls.loader.backup_registry()

from .models import DummyZip, GeoModelTest, RetailMachine

cls.loader.update_registry((GeoModelTest, DummyZip, RetailMachine))
add_to_registry(cls.registry, GeoModelTest)
cls.registry._setup_models__(cls.env.cr, ["geo.model.test"])
cls.registry.init_models(
cls.env.cr, ["geo.model.test"], {"models_to_check": True}
)

add_to_registry(cls.registry, DummyZip)
cls.registry._setup_models__(cls.env.cr, ["dummy.zip"])
cls.registry.init_models(cls.env.cr, ["dummy.zip"], {"models_to_check": True})

add_to_registry(cls.registry, RetailMachine)
cls.registry._setup_models__(cls.env.cr, ["retail.machine"])
cls.registry.init_models(
cls.env.cr, ["retail.machine"], {"models_to_check": True}
)

cls.geo_model = cls.env["geo.model.test"].create({})
cls.env["dummy.zip"].create(
{
Expand Down Expand Up @@ -145,7 +160,10 @@ def setUpClass(cls):

@classmethod
def tearDownClass(cls):
cls.loader.restore_registry()
cls.addClassCleanup(cls.registry.__delitem__, "geo.model.test")
cls.addClassCleanup(cls.registry.__delitem__, "retail.machine")
cls.addClassCleanup(cls.registry.__delitem__, "dummy.zip")

super().tearDownClass()

def test_create_multipolygon_wkt_format(self):
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Add tests covering invalid WKB/WKT inputs to reflect the new fallback behavior in value_to_shape

These changes add new behavior in value_to_shape (forcing use_wkb=True, catching GEOSException, logging failures, and falling back from WKB → WKT → POINT(0 0)). Please add tests here that cover: (1) a hex WKB string that raises GEOSException and still yields a valid geometry (ideally confirming the POINT(0 0) fallback when WKT is also invalid); (2) a non-geo string that fails both WKB and WKT and uses the fallback point; and (3) a valid WKT string to ensure forcing use_wkb=True doesn’t regress existing WKT-only inputs.

Suggested implementation:

    def test_create_multipolygon_wkt_format(self):
        GeoModel = self.env["geo.model.test"]
        wkt_multipolygon = "MULTIPOLYGON(((0 0,0 1,1 1,0 0)))"

        rec = GeoModel.create(
            {
                "name": "wkt-multipolygon",
                "the_geom": wkt_multipolygon,
            }
        )

        # Ensure a valid geometry is created and it is a multipolygon
        self.assertTrue(rec.the_geom)
        self.assertEqual(rec.the_geom.geom_type, "MultiPolygon")

    def test_invalid_wkb_and_wkt_fallback_to_default_point(self):
        GeoModel = self.env["geo.model.test"]

        # This is an invalid hex WKB string and also not a valid WKT
        invalid_wkb_and_wkt = "DEADBEEF"

        rec = GeoModel.create(
            {
                "name": "invalid-wkb-wkt",
                "the_geom": invalid_wkb_and_wkt,
            }
        )

        # value_to_shape should catch GEOSException / parse errors and
        # fall back to the default POINT(0 0) geometry
        self.assertTrue(rec.the_geom)
        self.assertEqual(rec.the_geom.geom_type, "Point")
        self.assertEqual(getattr(rec.the_geom, "x", None), 0)
        self.assertEqual(getattr(rec.the_geom, "y", None), 0)

    def test_non_geo_string_falls_back_to_default_point(self):
        GeoModel = self.env["geo.model.test"]

        non_geo_value = "this-is-not-a-geometry"

        rec = GeoModel.create(
            {
                "name": "non-geo-string",
                "the_geom": non_geo_value,
            }
        )

        # Both WKB and WKT parsing should fail and we should still get
        # the default POINT(0 0) geometry
        self.assertTrue(rec.the_geom)
        self.assertEqual(rec.the_geom.geom_type, "Point")
        self.assertEqual(getattr(rec.the_geom, "x", None), 0)
        self.assertEqual(getattr(rec.the_geom, "y", None), 0)

    def test_valid_wkt_with_wkb_first_parsing(self):
        GeoModel = self.env["geo.model.test"]

        # Valid WKT that should still be correctly parsed even though
        # value_to_shape now tries WKB parsing first
        wkt_point = "POINT(1 2)"

        rec = GeoModel.create(
            {
                "name": "valid-wkt-point",
                "the_geom": wkt_point,
            }
        )

        self.assertTrue(rec.the_geom)
        self.assertEqual(rec.the_geom.geom_type, "Point")
        self.assertEqual(getattr(rec.the_geom, "x", None), 1)
        self.assertEqual(getattr(rec.the_geom, "y", None), 2)

    def test_search_within_for_retails_34(self):
  1. If rec.the_geom is not a GEOS geometry but a different wrapper, you may need to adapt the assertions to the existing test style in this file (for example, accessing coordinates via rec.the_geom.coords[0] or rec.the_geom[0] instead of .x/.y and possibly using another property instead of .geom_type).
  2. Ensure that the "the_geom" field name and the "name" field exist on geo.model.test, or adjust them to match the model’s actual schema used elsewhere in test_model.py.
  3. If your test suite uses a different fixture or factory to create geo.model.test records, replace direct create calls with that helper for consistency.

Expand Down Expand Up @@ -481,7 +499,7 @@ def test_search_contains_for_retails_21(self):

def test_search_within_for_retails_34(self):
retails = self.env["retail.machine"]
zip_item = self.env["dummy.zip"].search([("city", "ilike", "Yens")])
zip_item = self.env["dummy.zip"].search([("city", "=", "Yens")], limit=1)
Copy link

@sourcery-ai sourcery-ai bot Jan 28, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Strengthen the test by asserting the setup data (zip_item) is found before using it

With the stricter domain, if dummy.zip ever stops containing a record with city "Yens", zip_item will be empty and the test may give a misleading result. Add an explicit assertion like self.assertTrue(zip_item, "Expected dummy.zip record for city 'Yens'") before accessing zip_item.the_geom so the test fails early and clearly when the setup data is missing.

result = retails.search(
[("name", "ilike", "34"), ("the_point", "geo_within", zip_item.the_geom)]
)
Expand Down
Loading