[XWI-133] add queries to fetch page hierachy information by Kharonus · Pull Request #24048 · opf/openproject

Kharonus · 2026-07-01T13:27:37Z

Ticket

What approach did you choose and why?

add two internal queries
add contracts, input and result model
HINT: this PR is a first step of https://community.openproject.org/wp/XWI-107
next step is amending the search service

- https://community.openproject.org/wp/XWI-133 - add two internal queries - add contracts, input and result model - HINT: this PR is a first step of https://community.openproject.org/wp/XWI-107 - next step is amending the search service

github-actions · 2026-07-01T13:28:06Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/wp/XWI-133
Work package version: not set
Core version: 17.7.0

Please make sure that:

The work package version OR your pull request target branch is correct

Kharonus · 2026-07-01T13:31:02Z

+    module Providers
+      module Internal
+        module Queries
+          module Internal


I hate it. And here I give the 🎤 to @NobodysNightmare and he can tell me: "Told you so."

What is happening? I voted to use a namespace called "internal" for queries and commands, that are not exposed via the registry. But I completely overlooked, that we already have an internal namespace for the provider type.

So, I'm ready to revisit that decision. WDYT? Shall I move all (2) queries out of this namespace and basically we rely that not exposing those query via the registry is the only indicator, that those are considered for "internal" use only?

I'd be fine to move internal queries out of the Internal namespace to avoid confusion.

Mh. Wait a minute. This PR exposes a problem with PRs that are too small: I can't see how those queries are intended to be used, there are no callers to them.

And this is kinda crucial here, because I am starting to get confused. Apparently you are planning for the PageHierarchy query to be internal. That would mean they are only used "inside" the provider. On the other hand, I know that they will be used for a feature that works across providers.

To me those two things don't line up yet. Either an outside caller wants to call those queries (making them non-internal) or another query wants to use them.

Do you have a preview of the PR that intends to use the queries somewhere already?

No, those are indeed internal. They will be used from the search_pages query. When searching, we no longer return a list of page infos, but a list of page hierarchies then.

Moved out of internal namespace

Kharonus · 2026-07-01T13:32:28Z

Missing tests ...

github-actions · 2026-07-01T13:46:39Z

Warning

Flaky specs

rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24048, linked for reference only):

- `rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]`

Treat this as a standalone task, unrelated to PR #24048. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24048 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @Kharonus to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @Kharonus, and request a review from @Kharonus.
On every commit, set @Kharonus as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

NobodysNightmare · 2026-07-02T11:21:27Z

+                  )
+                end
+
+                def wiki_result_from_wiki(wiki, provider:)


🟡 This method name is ambiguous, though I understand why you chose it.

The thing is that in other places we talk about a "page info result", when we don't return a PageInfo, but a Dry::Result(PageInfo). It is a hint at the fact that you don't get the thing directly, but there is a result to unpack first.

This is not the case here. Here we wanted to clarify that you don't get a Wiki (the AR model), but a ...::Results::Wiki`. Short: a wiki result.

It would also sound weird to call this method wiki_from_wiki.

Another thing I notice is that the other methods are called *_to_*, but this one is called *_from_*.

So yeah. I don't have good ideas, but take these bad ones:

ar_wiki_to_wiki

activerecord_wiki_to_wiki

wiki_model_to_wiki

wiki_to_adapter_wiki

Yes, I agree. Let me choose a better name

Hell, I chose one of the bad ones :D

NobodysNightmare · 2026-07-02T11:23:46Z

+                    identifier: wiki.id.to_s,
+                    provider:,
+                    name: wiki.project.name,
+                    # FIXME: internal wiki currently has no notion of a single root page, nor an official "entrypoint"


Alternatively we could link to the project's overview page. We take its name as well, so why not also taking its href?

In many cases the "first main page" will already be linked in the hierarchy anyways (one level deeper).

I agree, in fact, the URL is not used yet anywhere. And we did say, that we want to encourage the understanding of "the project is the wiki".

will change

NobodysNightmare · 2026-07-02T11:27:45Z

+
+              def call(input_data:, auth_strategy:)
+                Adapters::Authentication[auth_strategy].call do |user|
+                  wiki_page = WikiPage.visible(user).find_by(id: input_data.identifier)


🟡 Mh. Do we have any chance to include(:ancestors) or will this not fly, because that's a recursive relation?

it's recursive, if you execute that relation you see multiple SQL statements in the logs, all the same fetching the next parent.

NobodysNightmare · 2026-07-02T12:06:03Z

                json_hash.fetch(key) { throw :xwiki_error, Failure(Results::Error.new(source: self, code: :invalid_response)) }
              end
+
+              def dig_json(json_hash, *keys)


❗ Uh this is interesting. In a very recent PR of mine (#24049) I decided against the usage of dig, because it didn't behave the way I wanted it to.

I am leaving this here for you to decide, but hear me out :D

Essentially dig looks nice to go into unknown data at first, because it works well with the expected data as well as with unexpected data and it even mixes arrays and hashes:

{ a: [{ c: 42 }] }.dig(:a, 0, :c) => 42 { a: [{ c: 42 }] }.dig(:a, 100, :c) => nil { a: [{ c: 42 }] }.dig(:a, 0, :d) => nil

However, as soon as the structure mismatches in unexpected ways, you start receiving weird errors:

{ a: [{ c: 42 }] }.dig(:a, :b, :c) (irb):1:in 'Hash#dig': no implicit conversion of Symbol into Integer (TypeError)

This means error handling gets weird. You have to react to both nil responses and exceptions. That of course combines with the usual weakness of not being able to distinguish a "real" nil and a missing value.

I agree, I can make a more sophisticated implementation of the base class method.

took your approach and ported it to xwiki requests concern.

NobodysNightmare · 2026-07-02T12:41:24Z

+                def json_to_ancestors(data, provider:)
+                  page_title = fetch_json(data, "title")
+
+                  invalid_hierarchy_types = %w[document wiki]


What are the valid hierarchy types? Should we maybe use an allow list here?

Since we only expect pages/spaces, I can't imagine that it's an open list of valid types.

hmm, I'm only interested in type == space here, so I can swap the expectation. This grew from slightly different code.

NobodysNightmare · 2026-07-02T12:44:19Z

+                  ancestors = dig_json(data, "hierarchy", "items").filter do |item|
+                    next false if invalid_hierarchy_types.include?(fetch_json(item, "type"))
+
+                    fetch_json(item, "label") != page_title


🔴 Is that a valid condition?

What if I have a hierarchy like

Overview

Rooms

Storage Room

Overview

This hierarchy now contains "Overview" twice. Of course that's not great, but I also wouldn't know why we should prevent it.

Is this to filter the reference to the page itself? Couldn't this be solved by dropping the first/last element of the hierarchy? (probably both, because we filter wikis as well ^^)

Ah, crap.... this is bad, because right now I only have the name. Yeah, I need the identifier here, too.

But no, the reason why I exclude ancestors with the same title as the page, is that the WebHome appears in the ancestors, too, and it has the same label as the "leaf".

All WebHome? I am still wondering whether we can (safely and reliably) do things like "skip last element" or "skip first element".

But probably not, because a "Leaf Space" would have a WebHome and a "Leaf Final Page" would not, right?

exactly, I found both examples. :/

…iki-page

github-actions · 2026-07-06T13:01:56Z

Warning

Flaky specs

rspec ./spec/features/projects/create_spec.rb[1:12:3:3:1:1]
rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24048, linked for reference only):

- `rspec ./spec/features/projects/create_spec.rb[1:12:3:3:1:1]`
- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #24048. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24048 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @Kharonus to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @Kharonus, and request a review from @Kharonus.
On every commit, set @Kharonus as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Kharonus requested a review from a team July 1, 2026 13:27

Kharonus self-assigned this Jul 1, 2026

Kharonus commented Jul 1, 2026

View reviewed changes

NobodysNightmare reviewed Jul 2, 2026

View reviewed changes

NobodysNightmare requested changes Jul 2, 2026

View reviewed changes

Kharonus added 2 commits July 6, 2026 12:11

Merge branch 'dev' into implementation/xwi-133-fetch-ancestors-of-a-w…

f8a0dde

…iki-page

[XWI-133] move internal queries out of internal namespace

6b36ff8

Uh oh!

Conversation

Kharonus commented Jul 1, 2026

Ticket

What approach did you choose and why?

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kharonus commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kharonus Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Kharonus Jul 3, 2026 •

edited

Loading