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

Children() not returning items in the right order #17844

Open
enkelmedia opened this issue Dec 18, 2024 · 10 comments · May be fixed by #17898
Open

Children() not returning items in the right order #17844

enkelmedia opened this issue Dec 18, 2024 · 10 comments · May be fixed by #17898
Assignees
Labels
state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks type/bug

Comments

@enkelmedia
Copy link
Contributor

enkelmedia commented Dec 18, 2024

Which Umbraco version are you using?

15.1.1

Bug summary

After inspirational information from @bergmania about the performance of the new Hybrid Cache I decided to spend some time migrating a site that have about 10k nodes. Impressive improvements, startup time -30-40% and memory footprint -50%.

The first thing i noticed was that the menu was not sorted in the same way as in the backoffice. We create the menu like this:

var items = startModel.Children().Where(x => x.IsVisible());

The order in which the nodes were returned is not the same as in the backoffice, but adding OrderBy(x=>x.SortOrder) solves the problem:

var items = startModel.Children().Where(x => x.IsVisible()).OrderBy(x=>x.SortOrder);

I did try to rebuild the database cache (Settings -> Published Status) and reload the memory cache.

Is this intended behavior or is something wrong? :)

Specifics

The site was upgraded from v13.5.1 and this worked fine.

Steps to reproduce

See above.

Expected result / actual result

No response


This item has been added to our backlog AB#47319

Copy link

Hi there @enkelmedia!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Dec 19, 2024
@Zeegaan
Copy link
Member

Zeegaan commented Dec 19, 2024

Talking with @elit0451 about this, this seems like a bug, but it's weird, as we do call GetOrderedChildren here:

childrenKeys = GetOrderedChildren(parentNode, structure, contentTypeKey).ToList();

Which returns by sortorder:

.OrderBy(childKey => structure[childKey].SortOrder)

Are you able to reproduce this on a clean install ? 😁

@bielu
Copy link
Contributor

bielu commented Dec 19, 2024

@Zeegaan if you retrieve from db(or cache), order will change, so most likely you should order after converting IPublishedContent, not before :), there is no warranty on order when retrieving from storage.

@enkelmedia
Copy link
Contributor Author

@Zeegaan I have not seen it before when playing with v15, this was my first upgrade. I don't have a clear reproduction case from a clean install at the moment.

@bielu
Copy link
Contributor

bielu commented Dec 19, 2024

@enkelmedia what storage for hybrid cache are you using? as if sql, there quite chance it does randomly order items :)

@enkelmedia
Copy link
Contributor Author

Prying special here, just ”vanilla” config as in installing the Umbraco Template with dotnet new and point to the SQL server db

@JoseMarcenaro
Copy link
Contributor

I got the same effect all over the place - unordered results from Children() - after upgrading a v13 SQL DB.
Then I published each parent item with descendants and it temporarilly fixed the issue... but it's still happening randomly.
Each time it happens, the solution is to republish (with descendants) the affected parent.
I am using v15.1.1 now, but the upgrade was done with v15.0.0

Will try to reproduce with a clean install using SQL server.

@bielu
Copy link
Contributor

bielu commented Jan 5, 2025

@JoseMarcenaro your description confirmed that what I expected that it is about order how it is stored in storage, fix should be pretty easy for it to switch when ordering happens

@JoseMarcenaro
Copy link
Contributor

Hi @bielu thanks for your quick response.
But I do not understand what exactly to do in order to avoid this to keep happening (it does, from time to time)
Thx

@bielu
Copy link
Contributor

bielu commented Jan 5, 2025

@JoseMarcenaro umbraco will need fix it, as they order that when retrieving keys, but sql or others providers might not preserve order of keys when retrieving values, they might return values in order how they were index in storage. so Ordering should happen in method Children(), but that's more complicated as if they used cache structure to paginate they need do ordering in both places.

@Zeegaan Zeegaan self-assigned this Jan 6, 2025
@Zeegaan Zeegaan linked a pull request Jan 6, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants