Opened 4 years ago

Closed 19 months ago

Last modified 19 months ago

#3235 closed defect (fixed)

Vect_cidx_find_all should not allow mixing GV_AREA with other geometries

Reported by: marisn Owned by: grass-dev@…
Priority: major Milestone: 7.6.2
Component: LibVector Version: svn-trunk
Keywords: Cc:
CPU: Unspecified Platform: Unspecified

Description

At the moment Vect_cidx_find_all accepts any type mask. This leads to unusable results in case if typemask allows both GV_LINES and GV_AREA in the output. Reason - Vect_cidx_find_next will return line ID for GV_LINES and area ID for GV_AREA. As both ID spaces are separate (thus might contain identical ID values), mixing them in a single output structure is a road to disaster.

The fix - Vect_cidx_find_all should fail to accept type mask matching GV_LINES and GV_AREA (other combinations?) simultaneously. Other option - change Vect_cidx_find_all to modify two structs - one with line IDs and other with area IDs.

Change History (13)

comment:1 in reply to:  description Changed 4 years ago by mmetz

Replying to marisn:

At the moment Vect_cidx_find_all accepts any type mask. This leads to unusable results in case if typemask allows both GV_LINES and GV_AREA in the output. Reason - Vect_cidx_find_next will return line ID for GV_LINES and area ID for GV_AREA. As both ID spaces are separate (thus might contain identical ID values), mixing them in a single output structure is a road to disaster.

That really is a disaster. The original implementation of the category index did not take into account that area categories are added to the category index. Probably because this is duplicate information, an area can only have category values if it has a centroid with category values.

The fix - Vect_cidx_find_all should fail to accept type mask matching GV_LINES and GV_AREA (other combinations?) simultaneously. Other option - change Vect_cidx_find_all to modify two structs - one with line IDs and other with area IDs.

The real fix would be to not add area category values to the category index at all and update modules accordingly. This would be a substantial change in the vector libraries, but it can be implemented without breaking compatibility with G7. That would be a bit of work but I think it is worth the effort because it avoids ID collision.

comment:2 Changed 4 years ago by martinl

Milestone: 7.2.17.2.2

comment:3 Changed 3 years ago by neteler

Milestone: 7.2.27.2.3

Ticket retargeted after milestone closed

comment:4 Changed 3 years ago by martinl

Milestone: 7.2.3

Ticket retargeted after milestone closed

comment:5 Changed 3 years ago by martinl

Milestone: 7.2.4

comment:6 Changed 19 months ago by martinl

Still relevant?

comment:7 in reply to:  6 Changed 19 months ago by marisn

Milestone: 7.2.48.0.0

Replying to martinl:

Still relevant?

Complex code doesn't fix itself by accident. Yes, still relevant.

comment:8 Changed 19 months ago by mmetz

Resolution: fixed
Status: newclosed

In 74446:

vectorlib: Vect_cidx_find_all() should not allow mixing GV_AREA with other geometries, fixes #3235

comment:9 Changed 19 months ago by mmetz

In 74447:

vectorlib: Vect_cidx_find_all() should not allow mixing GV_AREA with other geometries, fixes #3235 (backport trunk r74446)

comment:10 Changed 19 months ago by neteler

Milestone: 8.0.07.6.2

comment:11 Changed 19 months ago by mmetz

Milestone: 7.6.28.0.0

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

comment:12 in reply to:  11 ; Changed 19 months ago by neteler

Replying to mmetz:

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

Then the bug report needs to be reopened again since we didn't reach 8.0.0 yet. Say, closing it with changes applied to 7.6.2 while the milestone is 8 is confusing (for me)...

comment:13 in reply to:  12 Changed 19 months ago by mmetz

Milestone: 8.0.07.6.2

Replying to neteler:

Replying to mmetz:

Modules using Vect_cidx_find_all() are v.profile, v.out.ogr, v.db.select, v.net, v.edit, and none of them mixes GV_AREA with other geometries. A fatal error will occur if a new module does mix GV_AREA with other geometries when calling Vect_cidx_find_all() (programmer's error).

Then the bug report needs to be reopened again since we didn't reach 8.0.0 yet. Say, closing it with changes applied to 7.6.2 while the milestone is 8 is confusing (for me)...

Sorry, that was trac confusion because we were editing the same time.

Note: See TracTickets for help on using tickets.