Prevent passing invalid kwds to DateOffset constructors #18226

jbrockmendel · 2017-11-11T04:09:16Z

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jreback · 2017-11-11T13:06:38Z

pandas/tests/tseries/test_offsets.py

+
+# ---------------------------------------------------------------------
+
+offset_classes = [getattr(offsets, x) for x in dir(offsets)]


we already list the classes somewhere in this module iirc.

Looks like a fixture defined in conftest, but only used in test_offsets. Instead of checking for the correct types it assume that offsets.__all__ is exactly what it needs. Mind if I a) put it directly test_offsets and b) make it check explicitly to avoid future footgunning?

jreback · 2017-11-11T13:06:52Z

pandas/tests/tseries/test_offsets.py

+                  issubclass(x, DateOffset)]
+
+
+def test_valid_attributes():


parameterize this

jreback · 2017-11-11T13:08:02Z

add a new whatsnew note indicating which classes are now strict (you did this for 0.21.0 IIRC), same language

jbrockmendel · 2017-11-11T17:29:21Z

add a new whatsnew note indicating which classes are now strict (you did this for 0.21.0 IIRC), same language

Will do. The 0.21.0 note didn't list subclasses. Sure you want them listed this time?

jbrockmendel · 2017-11-11T17:32:08Z

Once we get #18224 sorted out, the assert n == int(n) will be done only once (and probably as a ValueError instead of assertion) in _BaseOffset.init

codecov · 2017-11-11T19:41:26Z

Codecov Report

Merging #18226 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18226      +/-   ##
==========================================
- Coverage   91.42%   91.39%   -0.04%     
==========================================
  Files         163      163              
  Lines       50064    50099      +35     
==========================================
+ Hits        45773    45787      +14     
- Misses       4291     4312      +21

Flag	Coverage Δ
#multiple	`89.2% <100%> (-0.02%)`	⬇️
#single	`40.38% <63.41%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.17% <100%> (+0.06%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.38% <0%> (-1.82%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3493aba...1666d44. Read the comment docs.

codecov · 2017-11-11T19:41:41Z

Codecov Report

Merging #18226 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18226      +/-   ##
==========================================
- Coverage   91.33%   91.32%   -0.02%     
==========================================
  Files         163      163              
  Lines       49752    49764      +12     
==========================================
+ Hits        45443    45446       +3     
- Misses       4309     4318       +9

Flag	Coverage Δ
#multiple	`89.12% <100%> (ø)`	⬆️
#single	`40.72% <79.41%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/tests/tseries/offsets/conftest.py	`97.14% <100%> (ø)`	⬆️
pandas/tseries/offsets.py	`97.03% <100%> (+0.02%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be66ef8...b4b9e15. Read the comment docs.

jbrockmendel · 2017-11-12T04:39:50Z

appveyor failure appears unrelated. will rebase+push

jreback · 2017-11-12T15:48:06Z

doc/source/whatsnew/v0.22.0.txt

@@ -45,6 +45,7 @@ Other API Changes
 - :class:`Timestamp` will no longer silently ignore unused or invalid ``tz`` or ``tzinfo`` keyword arguments (:issue:`17690`)
 - :class:`Timestamp` will no longer silently ignore invalid ``freq`` arguments (:issue:`5168`)
 - :class:`CacheableOffset` and :class:`WeekDay` are no longer available in the ``pandas.tseries.offsets`` module (:issue:`17830`)
+- Restricted DateOffset keyword arguments.  Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior.  Now, only valid arguments will be accepted. (:issue:`17176`,:issue:`18226`).


1 space after a period, space after the common between issues

jreback · 2017-11-12T15:48:28Z

pandas/tests/tseries/conftest.py

-import pandas.tseries.offsets as offsets
-
-
-@pytest.fixture(params=[getattr(offsets, o) for o in offsets.__all__])


this serves the entire sub-directory, why are you removing?

Because it isn't moved outside test_offsets and causes a "where the heck is this defined" reaction to a new reader. Will revert.

jreback · 2017-11-12T15:48:48Z

pandas/tests/tseries/test_offsets.py

@@ -41,6 +42,20 @@
 from pandas.tseries.holiday import USFederalHolidayCalendar


+offset_classes = [getattr(offsets, x) for x in dir(offsets)]


no, pls just use the fixture (you can certainly create more if needed)

jreback · 2017-11-12T15:49:08Z

pandas/tests/tseries/test_offsets.py

+# ---------------------------------------------------------------------
+
+month_classes = [x for x in offset_classes if
+                 issubclass(x, offsets.MonthOffset)]


I would just make these fixtures

jreback · 2017-11-12T15:49:32Z

pandas/tseries/offsets.py

@@ -163,6 +164,7 @@ def __add__(date):
    normalize = False

    def __init__(self, n=1, normalize=False, **kwds):
+        assert n == int(n)


huh? why are you putting asserts here

for this case I actually do like
self.n = self._validate_n(n)

which would raise on a non-integer convertible passed in (with a helpful message that includes the class name); that's the reason to make this a method)

I'll get rid of the asserts here, handle them later.

jreback · 2017-11-12T16:06:15Z

#18210 looks kind of a duplicate with this. pls only have 1 per thing open at a time.

jbrockmendel · 2017-11-12T16:34:49Z

#18210 looks kind of a duplicate with this

Removed the asserts, which should fix this perception. They are orthogonal.

jbrockmendel · 2017-11-12T16:35:42Z

Though you're right about too many PRs. I actually closed several last night for that reason.

jreback · 2017-11-12T16:38:53Z

Though you're right about too many PRs. I actually closed several last night for that reason.

not a problem. just sometimes we can lose focus.

…libs-offsets-inits

jbrockmendel · 2017-11-14T02:21:23Z

appveyor error just says 'Command exited with code 1'. Any idea how to get something more meaningful than that?

…libs-offsets-inits

jbrockmendel · 2017-11-14T19:18:35Z

hmm the appveyor error is in the same place as befire in test_ticks.test_Hour

…libs-offsets-inits

jreback · 2017-11-19T15:42:20Z

pandas/_libs/tslibs/offsets.pyx

@@ -394,6 +394,15 @@ class _BaseOffset(object):
        out = '<%s' % n_str + className + plural + self._repr_attrs() + '>'
        return out

+    def _validate_n(self, n):


can you add some tests to exercise this?

Sure. Got one test so far, will add a few more after addressing the current test failure (which is a bug that may need to be addressed as part of a Larger Fix).

…libs-offsets-inits

jreback · 2017-11-23T16:06:53Z

pandas/_libs/tslibs/offsets.pyx

+        except (ValueError, TypeError):
+            raise TypeError('`n` argument must be an integer')
+        if n != nint:
+            raise ValueError('`n` argument must be an integer')


show the type of what was passed (for 1st)
make this a more informative message (e.g. showing both values) (for 2nd one)

jreback · 2017-11-23T16:07:39Z

pandas/tests/tseries/offsets/test_offsets.py

+
+
+def test_validate_n_error():
+    with pytest.raises(TypeError):


can you add some more tests here, e.g. for passing floats != to the int

jreback · 2017-11-23T16:08:21Z

pandas/tseries/offsets.py

        self.normalize = normalize
        if startingMonth is None:
            startingMonth = self._default_startingMonth
        self.startingMonth = startingMonth

        self.kwds = {'startingMonth': startingMonth}
+        self._offset = timedelta(1)
+        self._use_relativedelta = False


should _use_relative_delta be a class variable?

and _offset ?

_use_relativedelta is defined at the class level, defaults to False. So setting again here is unnecessary.

The bigger issue with _relativedelta&_offset is that they are only actually relevant for DateOffset (i.e. not subclasses). It took me a while to figure this out (and in fact I screwed up several PRs back by changing BusinessFoo.offset --> BusinessFoo._offset "for consistency" without realizing the business version is used differently). My current thought is to separate out the relativedelta-using parts of DateOffset into a subclass that isnt inherited by everything else to avoid this confusion.

jreback · 2017-11-23T16:08:43Z

pandas/tseries/offsets.py

@@ -2201,6 +2211,13 @@ class Easter(DateOffset):
    """
    _adjust_dst = True

+    def __init__(self, n=1, normalize=False):
+        self.n = self._validate_n(n)
+        self.normalize = normalize


same (and for other ones you are adding)

jreback · 2017-11-23T16:36:16Z

pandas/_libs/tslibs/offsets.pyx

@@ -406,6 +406,17 @@ class _BaseOffset(object):
        # will raise NotImplementedError.
        return get_day_of_month(other, self._day_opt)

+    def _validate_n(self, n):
+        try:


can you add the doc-string that you did for #18210

jreback · 2017-11-23T16:48:12Z

lgtm. ping on green.

jbrockmendel · 2017-11-24T15:25:13Z

Ping

…libs-offsets-inits

jreback · 2017-11-25T15:21:52Z

rebase and ping on green. lgtm.

…libs-offsets-inits

jbrockmendel · 2017-11-25T20:24:44Z

Ping

jreback · 2017-11-25T20:58:04Z

thanks @jbrockmendel

jreback requested changes Nov 11, 2017

View reviewed changes

jreback added the Frequency DateOffsets label Nov 11, 2017

jbrockmendel mentioned this pull request Nov 11, 2017

Tslibs offsets immutable #18224

Closed

jreback requested changes Nov 12, 2017

View reviewed changes

jbrockmendel added 10 commits November 12, 2017 16:06

Patch __init__ to prevent passing invalid kwds

b9fe60e

cast n to integer, assert equality

f10a1c6

whatsnew note

807d769

parameterize tests, define fixture where it is used

687e3b7

fixup

d5443ca

exclude base classes from testing

5e5e0c0

exclude base classes from testing

17f7b5a

dummy commit to force CI

3834ef8

edits per reviewer suggestions

1c54e96

whatsnew whitespace

0e37a24

jbrockmendel force-pushed the tslibs-offsets-inits branch from abcde3b to 0e37a24 Compare November 13, 2017 00:08

jbrockmendel added 2 commits November 13, 2017 06:17

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

a572368

…libs-offsets-inits

whitespace fixup

d0ff381

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

409dbd0

…libs-offsets-inits

jbrockmendel added 3 commits November 18, 2017 17:32

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

d5b8302

…libs-offsets-inits

implement _validate_n method

b54e26b

test for _validate_n

fade4a2

jreback requested changes Nov 19, 2017

View reviewed changes

jbrockmendel mentioned this pull request Nov 22, 2017

Tslibs offsets validation #18210

Closed

4 tasks

jbrockmendel added 6 commits November 21, 2017 19:35

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

38c2238

…libs-offsets-inits

Raise TypeError, not ValueError

fe895ff

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

11ba1a9

…libs-offsets-inits

Catch ValueError in int(n)

bc90a19

fixup extra imports

0b3dca0

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

9c841fc

…libs-offsets-inits

jreback requested changes Nov 23, 2017

View reviewed changes

remove unnecessary re-definitions, add tests, improve error msg

adee395

jreback requested changes Nov 23, 2017

View reviewed changes

jorisvandenbossche changed the title ~~Patch __init__ to prevent passing invalid kwds~~ Prevent passing invalid kwds to DateOffset constructors Nov 23, 2017

Add docstring to validate_n

43dc17e

jreback added this to the 0.22.0 milestone Nov 23, 2017

jreback approved these changes Nov 23, 2017

View reviewed changes

fixup missing format

4ff8c22

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

d261be9

…libs-offsets-inits

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

b4b9e15

…libs-offsets-inits

jreback merged commit 06518b2 into pandas-dev:master Nov 25, 2017

jbrockmendel deleted the tslibs-offsets-inits branch November 25, 2017 21:13

jbrockmendel mentioned this pull request Nov 28, 2017

Inconsistent date range behaviour #1573

Closed

jbrockmendel mentioned this pull request Dec 19, 2017

Offsets Roundup #18854

Closed

39 tasks


		# ---------------------------------------------------------------------

		offset_classes = [getattr(offsets, x) for x in dir(offsets)]

		import pandas.tseries.offsets as offsets


		@pytest.fixture(params=[getattr(offsets, o) for o in offsets.__all__])

		@@ -41,6 +42,20 @@
		from pandas.tseries.holiday import USFederalHolidayCalendar


		offset_classes = [getattr(offsets, x) for x in dir(offsets)]

Prevent passing invalid kwds to DateOffset constructors #18226

Prevent passing invalid kwds to DateOffset constructors #18226

Conversation

jbrockmendel commented Nov 11, 2017 • edited Loading

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

jreback commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

codecov bot commented Nov 11, 2017

Codecov Report

codecov bot commented Nov 11, 2017 • edited Loading

Codecov Report

jbrockmendel commented Nov 12, 2017

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

Choose a reason for hiding this comment

jreback commented Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jbrockmendel commented Nov 14, 2017

jbrockmendel commented Nov 14, 2017

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

Choose a reason for hiding this comment

jreback commented Nov 23, 2017

jbrockmendel commented Nov 24, 2017

jreback commented Nov 25, 2017

jbrockmendel commented Nov 25, 2017

jreback commented Nov 25, 2017

jbrockmendel commented Nov 11, 2017 •

edited

Loading

codecov bot commented Nov 11, 2017 •

edited

Loading