Fix BRP query failing when specifying missing/invalid components #18871

splo · 2025-04-17T15:44:11Z

Objective

Fixes BRP query fails when specifying missing/invalid components #18869.

Solution

The issue was the ? after a Result raising the error, instead of treating it.
Instead it is handled with ok, and_then, map ...

Edit: I added the following logic.
On bevy/query remote requests, when strict is false:

Unregistered components in option and without are ignored.
Unregistered components in has are considered absent from the entity.
Unregistered components in components and with result in an empty response since they specify hard requirements.

I made the get_component_ids function return a AnyhowResult<(Vec<(TypeId, ComponentId)>, Vec<String>)> instead of the previous AnyhowResult<Vec<(TypeId, ComponentId)>>; that is I added the list of unregistered components.

Testing

I tested changes using the same procedure as in the linked issue:

cargo run --example server --features="bevy_remote"

In another terminal:

# Not strict:
$ curl -X POST http://localhost:15702 -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "method": "bevy/query", "id": 0, "params": { "data": { "components": [ "foo::bar::MyComponent" ] } } }'
{"jsonrpc":"2.0","id":0,"result":[]}

# Strict:
$ curl -X POST http://localhost:15702 -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "method": "bevy/query", "id": 0, "params": { "data": { "components": [ "foo::bar::MyComponent" ] }, "strict": true } }'
{"jsonrpc":"2.0","id":0,"error":{"code":-23402,"message":"Component `foo::bar::MyComponent` isn't registered or used in the world"}}

github-actions · 2025-04-17T15:44:24Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Henauxg

The proposed change looks fine and seems to be a step in the right direction, however, I think something more is needed.

The expected remote query result for the strict: false query should be to return an empty list of entities, since foo::bar::MyComponent doesn't exist:

{
  "jsonrpc": "2.0",
  "id": 0,
  "result": []
}

However with the proposed change, since no error is raised, all the existing entities are returned as a response:

{"jsonrpc":"2.0","id":0,"result":[{"components":{},"entity":4294967297},{"components":{},"entity":4294967298},{"components":{},"entity":4294967299},{"components":{},"entity":4294967300},{"components":{},"entity":4294967317},{"components":{},"entity":4294967302},{"components":{},"entity":4294967303},{"components":{},"entity":4294967304},{"components":{},"entity":4294967305},{"components":{},"entity":4294967306},{"components":{},"entity":4294967307},{"components":{},"entity":4294967308},{"components":{},"entity":4294967309},{"components":{},"entity":4294967312},{"components":{},"entity":4294967313},{"components":{},"entity":4294967314},{"components":{},"entity":4294967315},{"components":{},"entity":4294967316},{"components":{},"entity":4294967310},{"components":{},"entity":4294967311},{"components":{},"entity":4294967318},{"components":{},"entity":4294967320},{"components":{},"entity":4294967319},{"components":{},"entity":4294967321},{"components":{},"entity":4294967322},{"components":{},"entity":4294967323},{"components":{},"entity":4294967324},{"components":{},"entity":4294967325},{"components":{},"entity":4294967301},{"components":{},"entity":4294967296}]}

As a result of silently ignoring missing components, it seems like the query being built in process_remote_query_request has no data nor filter configured and as such matches all entities.

splo · 2025-04-21T12:24:22Z

I agree, it should return an empty array.
I just fear that the right fix wouldn't have time/priority before the 0.16 release.
At least with the current content of the code changes, the query doesn't fail, and an option query works as intented; the following query would return the correct result:

{
  "jsonrpc": "2.0",
  "method": "bevy/query",
  "id": 0,
  "params": {
    "data": {
      "option": [
        "foo::bar::MyComponent"
      ]
    }
  }
}

mockersf · 2025-04-21T14:06:47Z

I don't think avoiding the error and returning too many entities is a good fix here.

If the correct fix is larger, it can go in the 0.16.1

splo · 2025-04-21T14:31:07Z

Then the following migration guide item should be removed: https://bevyengine.org/learn/migration-guides/0-15-to-0-16/#add-strict-field-to-brp-bevy-query.

Henauxg · 2025-04-22T16:04:48Z

I responded in more details in #18869, I feel like the fix needs to be a bit more holistic

On bevy/query remote requests, when "strict" is false: - Unregistered components in "option" and "without" are ignored. - Unregistered components in "has" are considered absent from the entity. - Unregistered components in "components" and "with" result in an empty response since they specify hard requirements.

splo · 2025-04-29T17:20:32Z

I fixed the issue following @Henauxg's specs: #18869 (comment).
When encountering unregistered component paths and strict is true, nothing changed: an error is raised.
When encountering unregistered component paths and strict is false, it depends on the request parameter:

Unregistered components in option and without are ignored.
Unregistered components in has are considered absent from the entity.
Unregistered components in components and with result in an empty response since they specify hard requirements.

splo · 2025-04-29T17:38:21Z

I don't think the build failed because of my changes. Maybe I rebased from a bad bevy/main version ...

Henauxg

I didn't test all the queries dynamically (just the components one), but the code looks good and does what we expect. If no one else raises their hand to spec it differently, it's fine with me.

Henauxg · 2025-05-02T15:07:11Z

crates/bevy_remote/src/builtin_methods.rs

    let component_ids = get_component_ids(&type_registry, world, components, true)
+        .and_then(|(registered, unregistered)| {
+            if unregistered.is_empty() {
+                Ok(registered)
+            } else {
+                Err(anyhow!("Unregistered component types: {:?}", unregistered))
+            }
+        })


Here, strict is set as true. This means unregistered will always be empty. Should we just simply return Ok(registered) ?

get_component_ids with strict true returns an empty unregistered vector for now but that may change later? It's just a sanity/robust check. It depends on the definition of get_component_ids ...
I have no strong feeling about this so if you insist, I'll change it.

No, no, I'm fine with robustness 👍

Henauxg · 2025-05-02T15:09:44Z

crates/bevy_remote/src/builtin_methods.rs

+    let (components, unregistered_in_components) =
+        get_component_ids(&type_registry, world, components, strict)
+            .map_err(BrpError::component_error)?;
+    let (option, _) = get_component_ids(&type_registry, world, option, strict)
        .map_err(BrpError::component_error)?;
-    let option = get_component_ids(&type_registry, world, option, strict)
-        .map_err(BrpError::component_error)?;
-    let has =
+    let (has, unregistered_in_has) =
        get_component_ids(&type_registry, world, has, strict).map_err(BrpError::component_error)?;
-    let without = get_component_ids(&type_registry, world, without, strict)
+    let (without, _) = get_component_ids(&type_registry, world, without, strict)
        .map_err(BrpError::component_error)?;
-    let with = get_component_ids(&type_registry, world, with, strict)
+    let (with, unregistered_in_with) = get_component_ids(&type_registry, world, with, strict)
        .map_err(BrpError::component_error)?;


It's a bit unfortunate that we retrieve the list of unregistered components type paths for all of them but only ever use the returned values within one of those lists for unregistered_in_has. But I guess doing otherwise would imply having specialized versions of get_component_ids, and I'm not sure that it's worth it.

Right, I prefered code simplicity and clarity than a small optimization. Again, if you insist, I'll change it with a specialized version.

Henauxg · 2025-05-02T19:24:15Z

(Removing X-Contentious as it's no longer the same proposed fix)

Trashtalk217

I've tested this locally, and I approve of the changes.

BenjaminBrienen · 2025-05-16T12:39:30Z

Please add a test(s) that demonstrates and ensures correct functionality.

splo · 2025-05-18T04:29:26Z

I agree tests are much needed for BRP-related stuff. I am not sure what form they should take.

Should we make actual HTTP requests? That would probably be the best but I believe that would be much work and should go in its own pull request.

If you see a way to make lightweight unit tests, please assist me.

) # Objective - Fixes #18869. ## Solution The issue was the `?` after a `Result` raising the error, instead of treating it. Instead it is handled with `ok`, `and_then`, `map` ... _Edit: I added the following logic._ On `bevy/query` remote requests, when `strict` is false: - Unregistered components in `option` and `without` are ignored. - Unregistered components in `has` are considered absent from the entity. - Unregistered components in `components` and `with` result in an empty response since they specify hard requirements. I made the `get_component_ids` function return a `AnyhowResult<(Vec<(TypeId, ComponentId)>, Vec<String>)>` instead of the previous `AnyhowResult<Vec<(TypeId, ComponentId)>>`; that is I added the list of unregistered components. ## Testing I tested changes using the same procedure as in the linked issue: ```sh cargo run --example server --features="bevy_remote" ``` In another terminal: ```sh # Not strict: $ curl -X POST http://localhost:15702 -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "method": "bevy/query", "id": 0, "params": { "data": { "components": [ "foo::bar::MyComponent" ] } } }' {"jsonrpc":"2.0","id":0,"result":[]} # Strict: $ curl -X POST http://localhost:15702 -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "method": "bevy/query", "id": 0, "params": { "data": { "components": [ "foo::bar::MyComponent" ] }, "strict": true } }' {"jsonrpc":"2.0","id":0,"error":{"code":-23402,"message":"Component `foo::bar::MyComponent` isn't registered or used in the world"}} ```

Henauxg reviewed Apr 21, 2025

View reviewed changes

NthTensor added this to the 0.16 milestone Apr 21, 2025

mockersf added the X-Contentious There are nontrivial implications that should be thought through label Apr 21, 2025

alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Apr 22, 2025

alice-i-cecile modified the milestones: 0.16, 0.16.1 Apr 22, 2025

Henauxg mentioned this pull request Apr 22, 2025

BRP query fails when specifying missing/invalid components #18869

Closed

splo added 2 commits April 29, 2025 19:06

Fix BRP query failing if component missing

d00892e

Merge branch 'bevyengine:main' into fix-18869

e067218

splo requested a review from Henauxg May 2, 2025 12:16

Henauxg approved these changes May 2, 2025

View reviewed changes

Henauxg removed the X-Contentious There are nontrivial implications that should be thought through label May 2, 2025

Trashtalk217 approved these changes May 16, 2025

View reviewed changes

Trashtalk217 removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 16, 2025

Trashtalk217 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 16, 2025

BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 16, 2025

BenjaminBrienen approved these changes May 18, 2025

View reviewed changes

BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 18, 2025

alice-i-cecile added this pull request to the merge queue May 26, 2025

alice-i-cecile mentioned this pull request May 26, 2025

Add tests for BRP #19371

Open

Merged via the queue into bevyengine:main with commit 2b8bf45 May 26, 2025
38 checks passed

splo deleted the fix-18869 branch May 26, 2025 16:47

Uh oh!

Fix BRP query failing when specifying missing/invalid components #18871

Fix BRP query failing when specifying missing/invalid components #18871

Uh oh!

Conversation

splo commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

Henauxg left a comment • 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!

splo commented Apr 21, 2025

Uh oh!

mockersf commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

splo commented Apr 21, 2025

Uh oh!

Henauxg commented Apr 22, 2025

Uh oh!

splo commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

splo commented Apr 29, 2025

Uh oh!

Henauxg left a comment

Choose a reason for hiding this comment

Uh oh!

Henauxg May 2, 2025 • 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!

splo May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Henauxg May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Henauxg May 2, 2025

Choose a reason for hiding this comment

Uh oh!

splo May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Henauxg commented May 2, 2025

Uh oh!

Trashtalk217 left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBrienen commented May 16, 2025

Uh oh!

splo commented May 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

splo commented Apr 17, 2025 •

edited

Loading

Henauxg left a comment •

edited

Loading

mockersf commented Apr 21, 2025 •

edited

Loading

splo commented Apr 29, 2025 •

edited

Loading

Henauxg May 2, 2025 •

edited

Loading