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"}} ```
This commit is contained in:
parent
8ff0c6481a
commit
ddc3264794
@ -718,17 +718,27 @@ pub fn process_remote_query_request(In(params): In<Option<Value>>, world: &mut W
|
|||||||
let app_type_registry = world.resource::<AppTypeRegistry>().clone();
|
let app_type_registry = world.resource::<AppTypeRegistry>().clone();
|
||||||
let type_registry = app_type_registry.read();
|
let type_registry = app_type_registry.read();
|
||||||
|
|
||||||
let components = get_component_ids(&type_registry, world, components, strict)
|
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)?;
|
.map_err(BrpError::component_error)?;
|
||||||
let option = get_component_ids(&type_registry, world, option, strict)
|
let (has, unregistered_in_has) =
|
||||||
.map_err(BrpError::component_error)?;
|
|
||||||
let has =
|
|
||||||
get_component_ids(&type_registry, world, has, strict).map_err(BrpError::component_error)?;
|
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)?;
|
.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)?;
|
.map_err(BrpError::component_error)?;
|
||||||
|
|
||||||
|
// 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.
|
||||||
|
if !unregistered_in_components.is_empty() || !unregistered_in_with.is_empty() {
|
||||||
|
return serde_json::to_value(BrpQueryResponse::default()).map_err(BrpError::internal);
|
||||||
|
}
|
||||||
|
|
||||||
let mut query = QueryBuilder::<FilteredEntityRef>::new(world);
|
let mut query = QueryBuilder::<FilteredEntityRef>::new(world);
|
||||||
for (_, component) in &components {
|
for (_, component) in &components {
|
||||||
query.ref_id(*component);
|
query.ref_id(*component);
|
||||||
@ -784,6 +794,7 @@ pub fn process_remote_query_request(In(params): In<Option<Value>>, world: &mut W
|
|||||||
let has_map = build_has_map(
|
let has_map = build_has_map(
|
||||||
row.clone(),
|
row.clone(),
|
||||||
has_paths_and_reflect_components.iter().copied(),
|
has_paths_and_reflect_components.iter().copied(),
|
||||||
|
&unregistered_in_has,
|
||||||
);
|
);
|
||||||
response.push(BrpQueryRow {
|
response.push(BrpQueryRow {
|
||||||
entity: row.id(),
|
entity: row.id(),
|
||||||
@ -1024,12 +1035,19 @@ pub fn process_remote_remove_request(
|
|||||||
let type_registry = app_type_registry.read();
|
let type_registry = app_type_registry.read();
|
||||||
|
|
||||||
let component_ids = get_component_ids(&type_registry, world, components, true)
|
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))
|
||||||
|
}
|
||||||
|
})
|
||||||
.map_err(BrpError::component_error)?;
|
.map_err(BrpError::component_error)?;
|
||||||
|
|
||||||
// Remove the components.
|
// Remove the components.
|
||||||
let mut entity_world_mut = get_entity_mut(world, entity)?;
|
let mut entity_world_mut = get_entity_mut(world, entity)?;
|
||||||
for (_, component_id) in component_ids {
|
for (_, component_id) in component_ids.iter() {
|
||||||
entity_world_mut.remove_by_id(component_id);
|
entity_world_mut.remove_by_id(*component_id);
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(Value::Null)
|
Ok(Value::Null)
|
||||||
@ -1264,8 +1282,9 @@ fn get_entity_mut(world: &mut World, entity: Entity) -> Result<EntityWorldMut<'_
|
|||||||
.map_err(|_| BrpError::entity_not_found(entity))
|
.map_err(|_| BrpError::entity_not_found(entity))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns the [`TypeId`] and [`ComponentId`] of the components with the given
|
/// Given components full path, returns a tuple that contains
|
||||||
/// full path names.
|
/// - A list of corresponding [`TypeId`] and [`ComponentId`] for registered components.
|
||||||
|
/// - A list of unregistered component paths.
|
||||||
///
|
///
|
||||||
/// Note that the supplied path names must be *full* path names: e.g.
|
/// Note that the supplied path names must be *full* path names: e.g.
|
||||||
/// `bevy_transform::components::transform::Transform` instead of `Transform`.
|
/// `bevy_transform::components::transform::Transform` instead of `Transform`.
|
||||||
@ -1274,25 +1293,33 @@ fn get_component_ids(
|
|||||||
world: &World,
|
world: &World,
|
||||||
component_paths: Vec<String>,
|
component_paths: Vec<String>,
|
||||||
strict: bool,
|
strict: bool,
|
||||||
) -> AnyhowResult<Vec<(TypeId, ComponentId)>> {
|
) -> AnyhowResult<(Vec<(TypeId, ComponentId)>, Vec<String>)> {
|
||||||
let mut component_ids = vec![];
|
let mut component_ids = vec![];
|
||||||
|
let mut unregistered_components = vec![];
|
||||||
|
|
||||||
for component_path in component_paths {
|
for component_path in component_paths {
|
||||||
let type_id = get_component_type_registration(type_registry, &component_path)?.type_id();
|
let maybe_component_tuple = get_component_type_registration(type_registry, &component_path)
|
||||||
let Some(component_id) = world.components().get_id(type_id) else {
|
.ok()
|
||||||
if strict {
|
.and_then(|type_registration| {
|
||||||
return Err(anyhow!(
|
let type_id = type_registration.type_id();
|
||||||
"Component `{}` isn't used in the world",
|
world
|
||||||
component_path
|
.components()
|
||||||
));
|
.get_id(type_id)
|
||||||
}
|
.map(|component_id| (type_id, component_id))
|
||||||
continue;
|
});
|
||||||
};
|
if let Some((type_id, component_id)) = maybe_component_tuple {
|
||||||
|
component_ids.push((type_id, component_id));
|
||||||
component_ids.push((type_id, component_id));
|
} else if strict {
|
||||||
|
return Err(anyhow!(
|
||||||
|
"Component `{}` isn't registered or used in the world",
|
||||||
|
component_path
|
||||||
|
));
|
||||||
|
} else {
|
||||||
|
unregistered_components.push(component_path);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(component_ids)
|
Ok((component_ids, unregistered_components))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Given an entity (`entity_ref`) and a list of reflected component information
|
/// Given an entity (`entity_ref`) and a list of reflected component information
|
||||||
@ -1325,12 +1352,16 @@ fn build_components_map<'a>(
|
|||||||
Ok(serialized_components_map)
|
Ok(serialized_components_map)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Given an entity (`entity_ref`) and list of reflected component information
|
/// Given an entity (`entity_ref`),
|
||||||
/// (`paths_and_reflect_components`), return a map which associates each component to
|
/// a list of reflected component information (`paths_and_reflect_components`)
|
||||||
/// a boolean value indicating whether or not that component is present on the entity.
|
/// and a list of unregistered components,
|
||||||
|
/// return a map which associates each component to a boolean value indicating
|
||||||
|
/// whether or not that component is present on the entity.
|
||||||
|
/// Unregistered components are considered absent from the entity.
|
||||||
fn build_has_map<'a>(
|
fn build_has_map<'a>(
|
||||||
entity_ref: FilteredEntityRef,
|
entity_ref: FilteredEntityRef,
|
||||||
paths_and_reflect_components: impl Iterator<Item = (&'a str, &'a ReflectComponent)>,
|
paths_and_reflect_components: impl Iterator<Item = (&'a str, &'a ReflectComponent)>,
|
||||||
|
unregistered_components: &[String],
|
||||||
) -> HashMap<String, Value> {
|
) -> HashMap<String, Value> {
|
||||||
let mut has_map = <HashMap<_, _>>::default();
|
let mut has_map = <HashMap<_, _>>::default();
|
||||||
|
|
||||||
@ -1338,6 +1369,9 @@ fn build_has_map<'a>(
|
|||||||
let has = reflect_component.contains(entity_ref.clone());
|
let has = reflect_component.contains(entity_ref.clone());
|
||||||
has_map.insert(type_path.to_owned(), Value::Bool(has));
|
has_map.insert(type_path.to_owned(), Value::Bool(has));
|
||||||
}
|
}
|
||||||
|
unregistered_components.iter().for_each(|component| {
|
||||||
|
has_map.insert(component.to_owned(), Value::Bool(false));
|
||||||
|
});
|
||||||
|
|
||||||
has_map
|
has_map
|
||||||
}
|
}
|
||||||
|
Loading…
Reference in New Issue
Block a user