| Message ID | 20210820145759.2180392-1-dja@axtens.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] REST: Don't error if a versioned field we would remove is absent | expand |
On Sat, Aug 21, 2021 at 12:57:58AM +1000, Daniel Axtens wrote: > This is odd, but not harmful and we definitely shouldn't 500. I can confirm that it fixes the 500 error and restores previous functionality. Thanks very much! Tested-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> -K
On Sat, 2021-08-21 at 00:57 +1000, Daniel Axtens wrote: > We remove fields that shouldn't be seen on old versions of the API. > This was done with `pop(field name)`, which will throw an exception > if the named field is absent from the data. However, sometimes if > a patch request is via an old API version, we hit this line without > ever having the field present. > > This is odd, but not harmful and we definitely shouldn't 500. > > Fixes: d944f17ec059 ("REST: Use versioning for modified responses") > Signed-off-by: Daniel Axtens <dja@axtens.net> Looks good to me. Reviewed-by: Stephen Finucane <stephen@that.guru> I squashed the test into this (with a tweak in the name) and applied it. Thanks! Stephen
On Fri, 20 Aug 2021 at 17:59, Stephen Finucane <stephen@that.guru> wrote: > > We remove fields that shouldn't be seen on old versions of the API. > > This was done with `pop(field name)`, which will throw an exception > > if the named field is absent from the data. However, sometimes if > > a patch request is via an old API version, we hit this line without > > ever having the field present. > > > > This is odd, but not harmful and we definitely shouldn't 500. > > > > Fixes: d944f17ec059 ("REST: Use versioning for modified responses") > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > Looks good to me. > > Reviewed-by: Stephen Finucane <stephen@that.guru> > > I squashed the test into this (with a tweak in the name) and applied it. Hi, all: Could this please be backported to stable/2.2 and maybe released as 2.2.6? -K
On Fri, 2021-10-22 at 08:28 -0400, Konstantin Ryabitsev wrote: > On Fri, 20 Aug 2021 at 17:59, Stephen Finucane <stephen@that.guru> wrote: > > > We remove fields that shouldn't be seen on old versions of the API. > > > This was done with `pop(field name)`, which will throw an exception > > > if the named field is absent from the data. However, sometimes if > > > a patch request is via an old API version, we hit this line without > > > ever having the field present. > > > > > > This is odd, but not harmful and we definitely shouldn't 500. > > > > > > Fixes: d944f17ec059 ("REST: Use versioning for modified responses") > > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > > > Looks good to me. > > > > Reviewed-by: Stephen Finucane <stephen@that.guru> > > > > I squashed the test into this (with a tweak in the name) and applied it. > > Hi, all: > > Could this please be backported to stable/2.2 and maybe released as 2.2.6? > > -K Hey, v2.2.6 is available now. Let me know if there are any issues. Somewhat related: are there any large blockers preventing you moving to v3.0.x? Is it the major version bump or removal of Python 2.7 support that's preventing the upgrade, or is there something else? In particular, is there anything that we could address? I'd like to get v3.1.0 out before the end of year but I'm slightly concerned that no one (?) has rolled forward to v3.0.x yet :D Cheers, Stephen
On Wed, Oct 27, 2021 at 05:00:34PM +0100, Stephen Finucane wrote: > v2.2.6 is available now. Let me know if there are any issues. Thanks! > Somewhat related: are there any large blockers preventing you moving to v3.0.x? > Is it the major version bump or removal of Python 2.7 support that's preventing > the upgrade, or is there something else? In particular, is there anything that > we could address? I'd like to get v3.1.0 out before the end of year but I'm > slightly concerned that no one (?) has rolled forward to v3.0.x yet :D The only concern, really, is the data migration and the potential for regressions. There are several subsystems now that are reliant on the CI aspects of patchwork.kernel.org to track their work, so I'm trying to be careful not to disrupt their work. I know that there are some cool new features in 3.x that would warrant migrating to it sooner than later -- just trying to plan out when is the best time to do it. On the upside, we're on the cusp of moving the web ui parts to a containerized deployment, so perhaps I can trial out the migration to 3.0 as part of that work. -K
Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes: > On Wed, Oct 27, 2021 at 05:00:34PM +0100, Stephen Finucane wrote: >> v2.2.6 is available now. Let me know if there are any issues. > > Thanks! > >> Somewhat related: are there any large blockers preventing you moving to v3.0.x? >> Is it the major version bump or removal of Python 2.7 support that's preventing >> the upgrade, or is there something else? In particular, is there anything that >> we could address? I'd like to get v3.1.0 out before the end of year but I'm >> slightly concerned that no one (?) has rolled forward to v3.0.x yet :D > > The only concern, really, is the data migration and the potential for > regressions. There are several subsystems now that are reliant on the CI > aspects of patchwork.kernel.org to track their work, so I'm trying to be > careful not to disrupt their work. I know that there are some cool new > features in 3.x that would warrant migrating to it sooner than later -- just > trying to plan out when is the best time to do it. > > On the upside, we're on the cusp of moving the web ui parts to a containerized > deployment, so perhaps I can trial out the migration to 3.0 as part of that > work. I'd probably hold off until I can get some of the pieces to make the migration more friendly in. I've tried to take on some of your feedback so it does things like migrate data in chunks rather than all in one giant statement... but up to you, of course! Who knows when I will get to them, work has just decided that we're going to bring forward a feature we had decided to defer so I am Busy! Kind regards, Daniel > > -K
diff --git a/patchwork/api/base.py b/patchwork/api/base.py index 89a43114f9e7..6cb703b12bbb 100644 --- a/patchwork/api/base.py +++ b/patchwork/api/base.py @@ -96,6 +96,9 @@ class BaseHyperlinkedModelSerializer(HyperlinkedModelSerializer): # field was added, we drop it if not utils.has_version(request, version): for field in self.Meta.versioned_fields[version]: - data.pop(field) + # After a PATCH with an older API version, we may not see + # these fields. If they don't exist, don't panic, return + # (and then discard) None. + data.pop(field, None) return data
We remove fields that shouldn't be seen on old versions of the API. This was done with `pop(field name)`, which will throw an exception if the named field is absent from the data. However, sometimes if a patch request is via an old API version, we hit this line without ever having the field present. This is odd, but not harmful and we definitely shouldn't 500. Fixes: d944f17ec059 ("REST: Use versioning for modified responses") Signed-off-by: Daniel Axtens <dja@axtens.net> --- patchwork/api/base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)