Skip to content

Commit ddc3264

Browse files
splomockersf
authored andcommitted
Fix BRP query failing when specifying missing/invalid components (#18871)
# 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"}} ```
1 parent 8ff0c64 commit ddc3264

File tree

1 file changed

+61
-27
lines changed

1 file changed

+61
-27
lines changed

crates/bevy_remote/src/builtin_methods.rs

Lines changed: 61 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -718,17 +718,27 @@ pub fn process_remote_query_request(In(params): In<Option<Value>>, world: &mut W
718718
let app_type_registry = world.resource::<AppTypeRegistry>().clone();
719719
let type_registry = app_type_registry.read();
720720

721-
let components = get_component_ids(&type_registry, world, components, strict)
721+
let (components, unregistered_in_components) =
722+
get_component_ids(&type_registry, world, components, strict)
723+
.map_err(BrpError::component_error)?;
724+
let (option, _) = get_component_ids(&type_registry, world, option, strict)
722725
.map_err(BrpError::component_error)?;
723-
let option = get_component_ids(&type_registry, world, option, strict)
724-
.map_err(BrpError::component_error)?;
725-
let has =
726+
let (has, unregistered_in_has) =
726727
get_component_ids(&type_registry, world, has, strict).map_err(BrpError::component_error)?;
727-
let without = get_component_ids(&type_registry, world, without, strict)
728+
let (without, _) = get_component_ids(&type_registry, world, without, strict)
728729
.map_err(BrpError::component_error)?;
729-
let with = get_component_ids(&type_registry, world, with, strict)
730+
let (with, unregistered_in_with) = get_component_ids(&type_registry, world, with, strict)
730731
.map_err(BrpError::component_error)?;
731732

733+
// When "strict" is false:
734+
// - Unregistered components in "option" and "without" are ignored.
735+
// - Unregistered components in "has" are considered absent from the entity.
736+
// - Unregistered components in "components" and "with" result in an empty
737+
// response since they specify hard requirements.
738+
if !unregistered_in_components.is_empty() || !unregistered_in_with.is_empty() {
739+
return serde_json::to_value(BrpQueryResponse::default()).map_err(BrpError::internal);
740+
}
741+
732742
let mut query = QueryBuilder::<FilteredEntityRef>::new(world);
733743
for (_, component) in &components {
734744
query.ref_id(*component);
@@ -784,6 +794,7 @@ pub fn process_remote_query_request(In(params): In<Option<Value>>, world: &mut W
784794
let has_map = build_has_map(
785795
row.clone(),
786796
has_paths_and_reflect_components.iter().copied(),
797+
&unregistered_in_has,
787798
);
788799
response.push(BrpQueryRow {
789800
entity: row.id(),
@@ -1024,12 +1035,19 @@ pub fn process_remote_remove_request(
10241035
let type_registry = app_type_registry.read();
10251036

10261037
let component_ids = get_component_ids(&type_registry, world, components, true)
1038+
.and_then(|(registered, unregistered)| {
1039+
if unregistered.is_empty() {
1040+
Ok(registered)
1041+
} else {
1042+
Err(anyhow!("Unregistered component types: {:?}", unregistered))
1043+
}
1044+
})
10271045
.map_err(BrpError::component_error)?;
10281046

10291047
// Remove the components.
10301048
let mut entity_world_mut = get_entity_mut(world, entity)?;
1031-
for (_, component_id) in component_ids {
1032-
entity_world_mut.remove_by_id(component_id);
1049+
for (_, component_id) in component_ids.iter() {
1050+
entity_world_mut.remove_by_id(*component_id);
10331051
}
10341052

10351053
Ok(Value::Null)
@@ -1264,8 +1282,9 @@ fn get_entity_mut(world: &mut World, entity: Entity) -> Result<EntityWorldMut<'_
12641282
.map_err(|_| BrpError::entity_not_found(entity))
12651283
}
12661284

1267-
/// Returns the [`TypeId`] and [`ComponentId`] of the components with the given
1268-
/// full path names.
1285+
/// Given components full path, returns a tuple that contains
1286+
/// - A list of corresponding [`TypeId`] and [`ComponentId`] for registered components.
1287+
/// - A list of unregistered component paths.
12691288
///
12701289
/// Note that the supplied path names must be *full* path names: e.g.
12711290
/// `bevy_transform::components::transform::Transform` instead of `Transform`.
@@ -1274,25 +1293,33 @@ fn get_component_ids(
12741293
world: &World,
12751294
component_paths: Vec<String>,
12761295
strict: bool,
1277-
) -> AnyhowResult<Vec<(TypeId, ComponentId)>> {
1296+
) -> AnyhowResult<(Vec<(TypeId, ComponentId)>, Vec<String>)> {
12781297
let mut component_ids = vec![];
1298+
let mut unregistered_components = vec![];
12791299

12801300
for component_path in component_paths {
1281-
let type_id = get_component_type_registration(type_registry, &component_path)?.type_id();
1282-
let Some(component_id) = world.components().get_id(type_id) else {
1283-
if strict {
1284-
return Err(anyhow!(
1285-
"Component `{}` isn't used in the world",
1286-
component_path
1287-
));
1288-
}
1289-
continue;
1290-
};
1291-
1292-
component_ids.push((type_id, component_id));
1301+
let maybe_component_tuple = get_component_type_registration(type_registry, &component_path)
1302+
.ok()
1303+
.and_then(|type_registration| {
1304+
let type_id = type_registration.type_id();
1305+
world
1306+
.components()
1307+
.get_id(type_id)
1308+
.map(|component_id| (type_id, component_id))
1309+
});
1310+
if let Some((type_id, component_id)) = maybe_component_tuple {
1311+
component_ids.push((type_id, component_id));
1312+
} else if strict {
1313+
return Err(anyhow!(
1314+
"Component `{}` isn't registered or used in the world",
1315+
component_path
1316+
));
1317+
} else {
1318+
unregistered_components.push(component_path);
1319+
}
12931320
}
12941321

1295-
Ok(component_ids)
1322+
Ok((component_ids, unregistered_components))
12961323
}
12971324

12981325
/// Given an entity (`entity_ref`) and a list of reflected component information
@@ -1325,19 +1352,26 @@ fn build_components_map<'a>(
13251352
Ok(serialized_components_map)
13261353
}
13271354

1328-
/// Given an entity (`entity_ref`) and list of reflected component information
1329-
/// (`paths_and_reflect_components`), return a map which associates each component to
1330-
/// a boolean value indicating whether or not that component is present on the entity.
1355+
/// Given an entity (`entity_ref`),
1356+
/// a list of reflected component information (`paths_and_reflect_components`)
1357+
/// and a list of unregistered components,
1358+
/// return a map which associates each component to a boolean value indicating
1359+
/// whether or not that component is present on the entity.
1360+
/// Unregistered components are considered absent from the entity.
13311361
fn build_has_map<'a>(
13321362
entity_ref: FilteredEntityRef,
13331363
paths_and_reflect_components: impl Iterator<Item = (&'a str, &'a ReflectComponent)>,
1364+
unregistered_components: &[String],
13341365
) -> HashMap<String, Value> {
13351366
let mut has_map = <HashMap<_, _>>::default();
13361367

13371368
for (type_path, reflect_component) in paths_and_reflect_components {
13381369
let has = reflect_component.contains(entity_ref.clone());
13391370
has_map.insert(type_path.to_owned(), Value::Bool(has));
13401371
}
1372+
unregistered_components.iter().for_each(|component| {
1373+
has_map.insert(component.to_owned(), Value::Bool(false));
1374+
});
13411375

13421376
has_map
13431377
}

0 commit comments

Comments
 (0)