U4-9435 - Fix breaking change made in U4-9312 - Changes signature of a couple back office controller actions

Created by Claus Jensen 24 Jan 2017, 19:01:21 Updated by Martin Griffiths 02 Feb 2017, 15:43:24

Tags: Unscheduled Regression

Relates to: U4-9436

Relates to: U4-9312

1 Attachments

Comments

Claus Jensen 24 Jan 2017, 19:03:05

PR: https://github.com/umbraco/Umbraco-CMS/pull/1703

Won't work as there's now multiple actions matching (it can't differentiate between the int/string type in parameters)

@Shandem any ideas?


Alex Lindgren 25 Jan 2017, 22:01:31

Is there a way to fix this without completely reverting it?


Claus Jensen 26 Jan 2017, 07:25:57

@Alex.Lindgren yep .. changing the name of the new method so it doesn't clash with the old one - and then keep the original signature for the old one.

As for a fix to anyone using the controller, whose code has broken in 7.5.8, the fix is easy as it's just making sure you do .ToString() on the int you're passing to the action.


Sebastiaan Janssen 26 Jan 2017, 07:28:48

That, unfortunately won't fix anything though.. cat's out of the bag now: people have already changed their code to call the method with ToString() now so if we remove that method again it will be another breaking change :-)


Sebastiaan Janssen 26 Jan 2017, 07:55:25

--This should do the trick: https://github.com/umbraco/Umbraco-CMS/pull/1706--


Claus Jensen 26 Jan 2017, 08:09:55

@sebastiaan Since you're doing exactly the same as I was in my original PR, I can assure you it won't ;)

You will have duplicated controller actions that it can't figure out how to resolve due to the param being sent being something like -1. It can't figure out whether to parse it as an int or a string when it is in a querystring.

Fix in https://github.com/umbraco/Umbraco-CMS/pull/1703 is still the only way to solve it.


Sebastiaan Janssen 26 Jan 2017, 09:19:44

I understand now what the problem was, WebApi calls didn't like the ambiguous method.

Since you now added an additional method, they don't collide any more and that's great! I've just added an additional method to help people who have already updated their code to work around the original issue of the method signature change. So now if you already did .ToString() in your code, it will just work. It's not pretty but at least we're not breaking the same sites for a second time: https://github.com/umbraco/Umbraco-CMS/pull/1703/commits/5681b3aa946ed0b99fc686b1e9988cdd767a40aa


Sebastiaan Janssen 26 Jan 2017, 09:24:46

Realized that I made a new method that was only internally used, so made it private! https://github.com/umbraco/Umbraco-CMS/pull/1703/commits/c857d822d82d84e95a181d347e28ebe29121ffcc


Claus Jensen 26 Jan 2017, 10:22:58

@sebastiaan Your latest commits won't work.

  1. You didn't fix the MediaTypeController.GetAllowedChildren - only the MediaController

  2. This fix won't work, since anyone actually using those methods before would now still be getting a multiple actions error since we now have both actions available again. Only reason why we are not seeing it in the backoffice now is because we're calling the new GetChildrenByString method.

see this SO post on webapi and params - there's really no good way to do what you're proposing: http://stackoverflow.com/questions/14353466/overload-web-api-action-method-based-on-parameter-type


Sebastiaan Janssen 26 Jan 2017, 11:11:54

Good point, we might (as Shannon said) be able to use a custom binder for that parameter instead.


Shannon Deminick 27 Jan 2017, 05:22:47

I've 'fixed' this so that we can have 3 methods:

MediaTypeController.GetAllowedChildren(int contentId) MediaTypeController.GetAllowedChildren(Guid contentId) \MediaTypeController.GetAllowedChildren(string contentId)

MediaController.GetChildren(int id) MediaController.GetChildren(Guid id) MediaController.GetChildren(string id)

This is possible by using a custom ApiControllerActionSelector. This selector will check what type is being passed in from the client and will choose the correct method accordingly. This means that there is no breaking changes from 7.5.7 since the same signature exists and we don't have to change any of the client code. This also means that if someone updated their signature to string in 7.5.8 we won't break it in 7.5.9 (though the string ones are marked obsolete)

PR: https://github.com/umbraco/Umbraco-CMS/pull/1709


Shannon Deminick 27 Jan 2017, 06:27:50

This can be tested with PostMan by passing in integer's or Guids for the ids.


Claus Jensen 27 Jan 2017, 08:06:39

Nice fix with the ActionSelector :) Tested and it seems to work - merged in.


Priority: Major

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.5.8

Due in version: 7.5.9

Sprint: Sprint 51

Story Points:

Cycle: