Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entity resource getByIds doesn't work as expected #11448

Closed
bjarnef opened this issue Oct 21, 2021 · 8 comments
Closed

Entity resource getByIds doesn't work as expected #11448

bjarnef opened this issue Oct 21, 2021 · 8 comments

Comments

@bjarnef
Copy link
Contributor

bjarnef commented Oct 21, 2021

Which exact Umbraco version are you using? For example: 8.13.1 - don't just write v8

9.0.1

Bug summary

In v8 we could pass in an array to entityResource.getByIds() to fetch documents, medias etc.

entityResource.getByIds([1173, 1180, 1162, 3233], "Media")
      .then(function (entities) {
              $scope.images = entities;
      });

According to docs this should be an array:
https://github.com/umbraco/Umbraco-CMS/blob/v9/contrib/src/Umbraco.Web.UI.Client/src/common/resources/entity.resource.js#L224-L265

I tried with int ids, udis and Guid, but all seems to fail.

image

Specifics

No response

Steps to reproduce

Request some media items / nodes from id, udi or guid via a dashboard or similar.

entityResource.getByIds([1173, 1180, 1162, 3233], "Media")
                .then(function (entities) {
                    $scope.images = entities;
                });

Expected result / actual result

No response


This item has been added to our backlog AB#15137

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 21, 2021

I also suggested this a year ago so we can fetch content and media by id, udi and guid (not slim entities).
#9465

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 27, 2021

image

It seems to fail somewhere here: https://github.com/umbraco/Umbraco-CMS/blob/v9/contrib/src/Umbraco.Web.BackOffice/Controllers/EntityController.cs#L512

entityResource.getByIds() should be able to lookup entities from int[], Guid[] and Udi[].

@AndyButland
Copy link
Contributor

Looks like there might be an issue with the ParameterSwapControllerActionSelector attribute added to that class. That should be detecting which of the three GetByIds methods to use. I can see here you are passing int[] but it looks to have matched one of the other methods in the controller, hence the attempt to convert to Guid in the error message you are seeing.

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 27, 2021

@AndyButland I can't find something specific recently has changed in EntityController or ParameterSwapControllerActionSelectorAttribute.

For the specific request I am looking at now from MNTP it has the content nodes selected, but since the request fails it doesn't show nodes in the repeat using <umb-node-preview>.

image

image

It use the first Udi, but for some reason it then can't convert this to an Guid.
I think it is failing here: https://github.com/umbraco/Umbraco-CMS/blob/v9/contrib/src/Umbraco.Web.BackOffice/Controllers/EntityController.cs#L512

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 27, 2021

Btw. the first examples with int[] was some hardcoded ids from controllers for custom block views.
The latter one from core MNTP which store udis and lookup these from entity resource.

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 14, 2021

I cloned latest changed from v9/contrib branch and installed the starterkit package.

I could use the following the fetch media:

entityResource.getByIds([1172, 1173], "Media")
    .then(function (entities) {
        console.log("entities", entities);
    });

but when I use udi instead it throws an exception:

entityResource.getByIds(['umb://media/cf1ab8dcad0f4a8e974b87b84777b0d6', 'umb://media/eee91c05b2e84031a056dcd7f28eff89'], "Media")
    .then(function (entities) {
      console.log("entities", entities);
    });

image

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 14, 2021

I also noticed similar issues with other pickers in default starterkit installed:

Home

image

Products

image

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 14, 2021

However when I ran the site with debugging through IIS Express it started working.

image

Afterwards when running without debugging it seems to work just fine 🤔

@p-m-j p-m-j added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Nov 17, 2021
p-m-j pushed a commit that referenced this issue Nov 23, 2021
This doesn't actually work in the backoffice because of GH #11448

So lets fix that next.
p-m-j pushed a commit that referenced this issue Nov 23, 2021
p-m-j pushed a commit that referenced this issue Nov 23, 2021
ParameterSwapControllerActionSelectorAttribute - cached body survived between requests.
bergmania pushed a commit that referenced this issue Nov 25, 2021
* Added EntityController.GetUrlsByIds support for int & guid + update MNTP

Fixes issue with MNTP (for 8.18) in a partial view macro - GH #11631

Renamed GetUrlsByUdis to match, don't do this in v9 as it would be
breaking there, instead mark it obsolete.

TODO: v9 ensure integration test coverage, more painful here as no
WebApplicationFactory.

* Added partial test coverage for GetUrlsByIds.

This doesn't actually work in the backoffice because of GH #11448

So lets fix that next.

* Failing test demonstrating #11448

* Fix for #11448 getByIds doesn't work as expected.

ParameterSwapControllerActionSelectorAttribute - cached body survived between requests.

* Expand on sync vs async comment for future confused souls.

* Might aswell cache parsed json vs string for performance

* Make ParameterSwapControllerActionSelector remarks more accurate.

* Share deserialized request body between action constraint and model binder

* Be more defensive with RequestBodyAsJObject HttpContext item

Only store if deserialize success.
Don't assume key being present means can cast as JObject.

* Nest constant a little deeper.

* Final defensive tweak
@umbrabot umbrabot removed the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants