[Django] #37193: ogrinfo() does not validate num_features, so None and negative values give surprising results

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------
Reporter: Caleb Jeffery Teye | Type:
| Cleanup/optimization
Status: new | Component: GIS
Version: dev | Severity: Normal
Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
django.contrib.gis.utils.ogrinfo(data_source, num_features=10) is
documented (and named) as taking the number of features to display per
layer. Internally, it passes the argument straight into a list slice:

# django/contrib/gis/utils/ogrinfo.py
for j, feature in enumerate(layer[:num_features]):
...

Because the value isn't validated, Python's slicing rules leak through and
produce results that don't match what the parameter name implies:

>>> from django.contrib.gis.utils import ogrinfo
>>> ogrinfo("cities.shp", num_features=None) # prints *every* feature,
not none
>>> ogrinfo("cities.shp", num_features=-2) # prints all features
except the last two

A caller reasonably expecting num_features to mean "how many features to
show" gets neither an error nor the requested count.

ogrinfo is exported in __all__, so this is public API. The fix should
decide on the intended contract — most likely validating that num_features
is a non-negative integer and raising TypeError/ValueError otherwise — and
add tests. Any change in behaviour for None/negative input should be
weighed for backwards compatibility (possibly a deprecation path).

Spotted while documenting ogrinfo() in #25927 — see the review discussion
on [https://github.com/django/django/pull/21443 PR #21443].
--
Ticket URL: <https://code.djangoproject.com/ticket/37193>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------

Reporter: Caleb Jeffery Teye | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: dev
Severity: Normal | Resolution:

Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by David Smith:

Old description:

> django.contrib.gis.utils.ogrinfo(data_source, num_features=10) is
> documented (and named) as taking the number of features to display per
> layer. Internally, it passes the argument straight into a list slice:
>
> # django/contrib/gis/utils/ogrinfo.py
> for j, feature in enumerate(layer[:num_features]):
> ...
>
> Because the value isn't validated, Python's slicing rules leak through
> and produce results that don't match what the parameter name implies:
>
> >>> from django.contrib.gis.utils import ogrinfo
> >>> ogrinfo("cities.shp", num_features=None) # prints *every* feature,
> not none
> >>> ogrinfo("cities.shp", num_features=-2) # prints all features
> except the last two
>
> A caller reasonably expecting num_features to mean "how many features to
> show" gets neither an error nor the requested count.
>
> ogrinfo is exported in __all__, so this is public API. The fix should
> decide on the intended contract — most likely validating that
> num_features is a non-negative integer and raising TypeError/ValueError
> otherwise — and add tests. Any change in behaviour for None/negative
> input should be weighed for backwards compatibility (possibly a
> deprecation path).
>
> Spotted while documenting ogrinfo() in #25927 — see the review discussion
> on [https://github.com/django/django/pull/21443 PR #21443].

New description:

django.contrib.gis.utils.ogrinfo(data_source, num_features=10) is
documented (and named) as taking the number of features to display per
layer. Internally, it passes the argument straight into a list slice:

{{{

# django/contrib/gis/utils/ogrinfo.py
for j, feature in enumerate(layer[:num_features]):
...
}}}

Because the value isn't validated, Python's slicing rules leak through and
produce results that don't match what the parameter name implies:

{{{
>>> from django.contrib.gis.utils import ogrinfo
>>> ogrinfo("cities.shp", num_features=None) # prints *every* feature,
not none
>>> ogrinfo("cities.shp", num_features=-2) # prints all features
except the last two
}}}

A caller reasonably expecting num_features to mean "how many features to
show" gets neither an error nor the requested count.

ogrinfo is exported in __all__, so this is public API. The fix should
decide on the intended contract — most likely validating that num_features
is a non-negative integer and raising TypeError/ValueError otherwise — and
add tests. Any change in behaviour for None/negative input should be
weighed for backwards compatibility (possibly a deprecation path).

Spotted while documenting ogrinfo() in #25927 — see the review discussion
on [https://github.com/django/django/pull/21443 PR #21443].

--

--
Ticket URL: <https://code.djangoproject.com/ticket/37193#comment:1>

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------
Reporter: Caleb Jeffery Teye | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by David Smith):

What do you think about documenting it as part of #25927.

> `num_features` is used as a list slice to limit the number of features
returned. Typically, this is expected to be a non-negative integer.
--
Ticket URL: <https://code.djangoproject.com/ticket/37193#comment:2>

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------
Reporter: Caleb Jeffery Teye | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Caleb Jeffery Teye):

Replying to [comment:2 David Smith]:

> What do you think about documenting it as part of #25927.
>
> > `num_features` is used as a list slice to limit the number of features
returned. Typically, this is expected to be a non-negative integer.

Sounds good to me. Let's document it as part of #25927. I've added a note
to the `num_features` description:

> `num_features` sets how many features are listed per layer, and defaults
to 10. It is used as a list slice, so it should be a non-negative integer.

Once that lands, this ticket can be closed.
--
Ticket URL: <https://code.djangoproject.com/ticket/37193#comment:3>

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------
Reporter: Caleb Jeffery Teye | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by maheen8q):

I'd like to work on this if it's still available. My plan is to add
validation at the top of ogrinfo() that raises a ValueError for negative
integers, and a TypeError for non-integer values. I'll leave the None
behavior open for reviewer guidance given the backwards compatibility
concerns mentioned in the description, and will include tests for the new
validation.
--
Ticket URL: <https://code.djangoproject.com/ticket/37193#comment:4>

[Django]

#37193: ogrinfo() does not validate num_features, so None and negative values give
surprising results
-------------------------------------+-------------------------------------
Reporter: Caleb Jeffery Teye | Owner: (none)

Type: | Status: closed

Cleanup/optimization |
Component: GIS | Version: dev

Severity: Normal | Resolution: wontfix

Keywords: ogrinfo | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by David Smith):

* resolution: => wontfix
* status: new => closed

Comment:

Hi Caleb -- I'll close this ticket to try and avoid folk working on
something you've already well progressed. I'll leave a note on the other
ticket back to this as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/37193#comment:5>