From 8ee8fe86cc50c31f9c840a44defd6cbb3a30a80c Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Wed, 21 May 2025 14:57:52 +0000 Subject: [PATCH 1/9] Fix empty data on MapTraceEvent --- deebot_client/map.py | 3 ++- src/map.rs | 31 ++++++++++++++++++++----------- src/util.rs | 11 ++++++++--- tests/test_map.py | 15 ++++++++++++++- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/deebot_client/map.py b/deebot_client/map.py index 54f6dc00b..07e088c0e 100644 --- a/deebot_client/map.py +++ b/deebot_client/map.py @@ -138,7 +138,8 @@ async def on_map_trace(event: MapTraceEvent) -> None: if event.start == 0: self._map_data.clear_trace_points() - self._map_data.add_trace_points(event.data) + if event.data: + self._map_data.add_trace_points(event.data) unsubscribers.append(self._event_bus.subscribe(MapTraceEvent, on_map_trace)) diff --git a/src/map.rs b/src/map.rs index 28aa3173c..e788f1283 100644 --- a/src/map.rs +++ b/src/map.rs @@ -68,7 +68,7 @@ fn process_trace_points(trace_points: &[u8]) -> Result, Box Result, Box> { +fn extract_trace_points(value: &str) -> Result, Box> { let decompressed_data = decompress_base64_data(value)?; process_trace_points(&decompressed_data) } @@ -333,9 +333,13 @@ impl MapData { } fn add_trace_points(&mut self, value: String) -> Result<(), PyErr> { - self.trace_points.extend( - extract_trace_points(value).map_err(|err| PyValueError::new_err(err.to_string()))?, - ); + self.trace_points + .extend(extract_trace_points(&value).map_err(|err| { + let mut err = err.to_string(); + err.push_str(";value:"); + err.push_str(&value); + PyValueError::new_err(err) + })?); Ok(()) } @@ -348,8 +352,15 @@ impl MapData { return Err(PyValueError::new_err("Index out of bounds")); } self.map_pieces[index] - .update_points(base64_data) - .map_err(|err| PyValueError::new_err(err.to_string())) + .update_points(&base64_data) + .map_err(|err| { + let mut err = err.to_string(); + err.push_str(";index:"); + err.push_str(index.to_string().as_str()); + err.push_str(",base64_data:"); + err.push_str(&base64_data); + PyValueError::new_err(err) + }) } fn map_piece_crc32_indicates_update( @@ -627,7 +638,7 @@ impl MapPiece { self.pixels_indexed.as_ref() } - fn update_points(&mut self, base64_data: String) -> Result> { + fn update_points(&mut self, base64_data: &str) -> Result> { let decoded = decompress_base64_data(base64_data)?; let old_crc32 = self.crc32; @@ -795,7 +806,7 @@ mod tests { #[test] fn test_extract_trace_points_success() { let input = "XQAABACvAAAAAAAAAEINQkt4BfqEvt9Pow7YU9KWRVBcSBosIDAOtACCicHy+vmfexxcutQUhqkAPQlBawOeXo/VSrOqF7yhdJ1JPICUs3IhIebU62Qego0vdk8oObiLh3VY/PVkqQyvR4dHxUDzMhX7HAguZVn3yC17+cQ18N4kaydN3LfSUtV/zejrBM4="; - let result = extract_trace_points(input.to_string()).unwrap(); + let result = extract_trace_points(input).unwrap(); let expected = vec![ TracePoint { x: 0, @@ -985,9 +996,7 @@ mod tests { #[test] fn test_update_map_piece_of_empty_piece() { - let data = String::from( - "XQAABAAQJwAAAABv/f//o7f/Rz5IFXI5YVG4kijmo4YH+e7kHoLTL8U6PAFLsX7Jhrz0KgA=", - ); + let data = "XQAABAAQJwAAAABv/f//o7f/Rz5IFXI5YVG4kijmo4YH+e7kHoLTL8U6PAFLsX7Jhrz0KgA="; let mut map_piece = MapPiece { crc32: 0, pixels_indexed: None, diff --git a/src/util.rs b/src/util.rs index f5af2a9b9..78acd82a8 100644 --- a/src/util.rs +++ b/src/util.rs @@ -7,7 +7,7 @@ use liblzma::stream::Stream; use pyo3::exceptions::PyValueError; use pyo3::prelude::*; -pub fn decompress_base64_data(value: String) -> Result, Box> { +pub fn decompress_base64_data(value: &str) -> Result, Box> { let bytes = general_purpose::STANDARD.decode(value)?; if is_zstd_compressed(&bytes) { @@ -51,8 +51,13 @@ fn is_zstd_compressed(bytes: &[u8]) -> bool { /// Decompress base64 decoded compressed string by using lzma or zstd #[pyfunction(name = "decompress_base64_data")] -fn python_decompress_base64_data(value: String) -> Result, PyErr> { - decompress_base64_data(value).map_err(|err| PyValueError::new_err(err.to_string())) +fn python_decompress_base64_data(value: &str) -> Result, PyErr> { + decompress_base64_data(value).map_err(|err| { + let mut err = err.to_string(); + err.push_str(";value:"); + err.push_str(value); + PyValueError::new_err(err) + }) } pub fn init_module(m: &Bound<'_, PyModule>) -> PyResult<()> { diff --git a/tests/test_map.py b/tests/test_map.py index 4ef7c357d..c111917e8 100644 --- a/tests/test_map.py +++ b/tests/test_map.py @@ -2,7 +2,7 @@ import asyncio from typing import TYPE_CHECKING -from unittest.mock import ANY, AsyncMock, Mock, call +from unittest.mock import ANY, AsyncMock, Mock, call, patch import pytest @@ -188,6 +188,19 @@ async def test_get_svg_map_empty( assert map.get_svg_map() is None +async def test_empty_maptrace( + execute_mock: AsyncMock, + event_bus: EventBus, + static_device_info: StaticDeviceInfo, +) -> None: + """Test empty data will not raise exception.""" + with patch("deebot_client.map.MapData", autospec=True): + map = await setup_map(execute_mock, event_bus, static_device_info) + event_bus.notify(MapTraceEvent(0, 0, "")) + await block_till_done(event_bus) + map._map_data.add_trace_points.assert_not_called() + + def test_get_svg_map( event_loop: asyncio.AbstractEventLoop, benchmark: BenchmarkFixture, From cfd27095b72a43f14b45e8b317574885d7243881 Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Wed, 21 May 2025 18:54:17 +0200 Subject: [PATCH 2/9] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/map.rs | 10 ++-------- src/util.rs | 4 +--- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/map.rs b/src/map.rs index e788f1283..dd9d80b27 100644 --- a/src/map.rs +++ b/src/map.rs @@ -335,9 +335,7 @@ impl MapData { fn add_trace_points(&mut self, value: String) -> Result<(), PyErr> { self.trace_points .extend(extract_trace_points(&value).map_err(|err| { - let mut err = err.to_string(); - err.push_str(";value:"); - err.push_str(&value); + let err = format!("{};value:{}", err, value); PyValueError::new_err(err) })?); Ok(()) @@ -354,11 +352,7 @@ impl MapData { self.map_pieces[index] .update_points(&base64_data) .map_err(|err| { - let mut err = err.to_string(); - err.push_str(";index:"); - err.push_str(index.to_string().as_str()); - err.push_str(",base64_data:"); - err.push_str(&base64_data); + let err = format!("{};index:{},base64_data:{}", err, index, base64_data); PyValueError::new_err(err) }) } diff --git a/src/util.rs b/src/util.rs index 78acd82a8..d0ab6fb61 100644 --- a/src/util.rs +++ b/src/util.rs @@ -53,9 +53,7 @@ fn is_zstd_compressed(bytes: &[u8]) -> bool { #[pyfunction(name = "decompress_base64_data")] fn python_decompress_base64_data(value: &str) -> Result, PyErr> { decompress_base64_data(value).map_err(|err| { - let mut err = err.to_string(); - err.push_str(";value:"); - err.push_str(value); + let err = format!("{};value:{}", err, value); PyValueError::new_err(err) }) } From 9b589a926eda8567ac50d0a2e348d04b2166bb8d Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Wed, 21 May 2025 17:16:39 +0000 Subject: [PATCH 3/9] Add tests for invalid data --- src/map.rs | 5 ++++- tests/rs/test_map.py | 46 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 tests/rs/test_map.py diff --git a/src/map.rs b/src/map.rs index dd9d80b27..1dc627c8f 100644 --- a/src/map.rs +++ b/src/map.rs @@ -347,7 +347,10 @@ impl MapData { fn update_map_piece(&mut self, index: usize, base64_data: String) -> Result { if index >= self.map_pieces.len() { - return Err(PyValueError::new_err("Index out of bounds")); + return Err(PyValueError::new_err(format!( + "Index out of bounds;index:{},base64_data:{}", + index, base64_data + ))); } self.map_pieces[index] .update_points(&base64_data) diff --git a/tests/rs/test_map.py b/tests/rs/test_map.py new file mode 100644 index 000000000..bf45d2781 --- /dev/null +++ b/tests/rs/test_map.py @@ -0,0 +1,46 @@ +"""Test rust map module.""" + +from __future__ import annotations + +import pytest + +from deebot_client.rs.map import MapData + + +@pytest.mark.parametrize( + ("value", "expected_error"), + [ + ("invalid_base64", "Invalid symbol 95, offset 7.;value:invalid_base64"), + ("", "Invalid 7z compressed data;value:"), + ], +) +def test_MapData_add_trace_points_invalid(value: str, expected_error: str) -> None: + """Test invalid MapData.add_trace_points.""" + map_data = MapData() + with pytest.raises(ValueError, match=expected_error): + map_data.add_trace_points(value) + + +@pytest.mark.parametrize( + ("index", "base64_data", "expected_error"), + [ + ( + 10000, + "invalid_base64", + "Index out of bounds;index:10000,base64_data:invalid_base64", + ), + ( + 1, + "invalid_base64", + "Invalid symbol 95, offset 7.;index:1,base64_data:invalid_base64", + ), + (1, "", "Invalid 7z compressed data;index:1,base64_data:"), + ], +) +def test_MapData_update_map_piece_invalid( + index: int, base64_data: str, expected_error: str +) -> None: + """Test invalid MapData.update_map_piece.""" + map_data = MapData() + with pytest.raises(ValueError, match=expected_error): + map_data.update_map_piece(index, base64_data) From 2f20d3a80168f329d87493b86aa83df44040b3d5 Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Thu, 22 May 2025 09:40:27 +0200 Subject: [PATCH 4/9] Update deebot_client/map.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- deebot_client/map.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deebot_client/map.py b/deebot_client/map.py index 07e088c0e..36b9f816a 100644 --- a/deebot_client/map.py +++ b/deebot_client/map.py @@ -138,7 +138,7 @@ async def on_map_trace(event: MapTraceEvent) -> None: if event.start == 0: self._map_data.clear_trace_points() - if event.data: + if event.data.strip(): self._map_data.add_trace_points(event.data) unsubscribers.append(self._event_bus.subscribe(MapTraceEvent, on_map_trace)) From a32203a4b60aa3c83c3a78359c78cd57bb91c957 Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Thu, 22 May 2025 07:52:21 +0000 Subject: [PATCH 5/9] Log instead --- src/map.rs | 21 ++++++++++------- src/util.rs | 5 ++-- tests/rs/test_map.py | 54 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 59 insertions(+), 21 deletions(-) diff --git a/src/map.rs b/src/map.rs index 1dc627c8f..46be5b9a9 100644 --- a/src/map.rs +++ b/src/map.rs @@ -7,7 +7,7 @@ use base64::Engine; use byteorder::{LittleEndian, ReadBytesExt}; use crc32fast::Hasher; use image::{GenericImageView, GrayImage, Luma}; -use log::debug; +use log::{debug, error}; use once_cell::sync::Lazy; use png::{BitDepth, ColorType, Compression, Encoder}; use pyo3::exceptions::PyValueError; @@ -335,8 +335,8 @@ impl MapData { fn add_trace_points(&mut self, value: String) -> Result<(), PyErr> { self.trace_points .extend(extract_trace_points(&value).map_err(|err| { - let err = format!("{};value:{}", err, value); - PyValueError::new_err(err) + error!("Failed to extract trace points: {};value:{}", err, value); + PyValueError::new_err(err.to_string()) })?); Ok(()) } @@ -347,16 +347,20 @@ impl MapData { fn update_map_piece(&mut self, index: usize, base64_data: String) -> Result { if index >= self.map_pieces.len() { - return Err(PyValueError::new_err(format!( - "Index out of bounds;index:{},base64_data:{}", + error!( + "Index out of bounds; index:{}, base64_data:{}", index, base64_data - ))); + ); + return Err(PyValueError::new_err("Index out of bounds")); } self.map_pieces[index] .update_points(&base64_data) .map_err(|err| { - let err = format!("{};index:{},base64_data:{}", err, index, base64_data); - PyValueError::new_err(err) + error!( + "Failed to update map piece: {}; index:{}, base64_data:{}", + err, index, base64_data, + ); + PyValueError::new_err(err.to_string()) }) } @@ -366,6 +370,7 @@ impl MapData { crc32: u32, ) -> Result { if index >= self.map_pieces.len() { + error!("Index out of bounds; index:{}", index); return Err(PyValueError::new_err("Index out of bounds")); } Ok(self.map_pieces[index].crc32_indicates_update(crc32)) diff --git a/src/util.rs b/src/util.rs index d0ab6fb61..61199c2aa 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,3 +1,4 @@ +use log::error; use std::error::Error; use std::io::{Cursor, Read}; @@ -53,8 +54,8 @@ fn is_zstd_compressed(bytes: &[u8]) -> bool { #[pyfunction(name = "decompress_base64_data")] fn python_decompress_base64_data(value: &str) -> Result, PyErr> { decompress_base64_data(value).map_err(|err| { - let err = format!("{};value:{}", err, value); - PyValueError::new_err(err) + error!("Error decompressing base64 data: {}; value:{}", err, value); + PyValueError::new_err(err.to_string()) }) } diff --git a/tests/rs/test_map.py b/tests/rs/test_map.py index bf45d2781..a41230d37 100644 --- a/tests/rs/test_map.py +++ b/tests/rs/test_map.py @@ -3,44 +3,76 @@ from __future__ import annotations import pytest +from testfixtures import LogCapture from deebot_client.rs.map import MapData @pytest.mark.parametrize( - ("value", "expected_error"), + ("value", "expected_error", "expected_log"), [ - ("invalid_base64", "Invalid symbol 95, offset 7.;value:invalid_base64"), - ("", "Invalid 7z compressed data;value:"), + ( + "invalid_base64", + "Invalid symbol 95, offset 7.", + "Failed to extract trace points: Invalid symbol 95, offset 7.;value:invalid_base64", + ), + ( + "", + "Invalid 7z compressed data", + "Failed to extract trace points: Invalid 7z compressed data;value:", + ), ], ) -def test_MapData_add_trace_points_invalid(value: str, expected_error: str) -> None: +def test_MapData_add_trace_points_invalid( + value: str, expected_error: str, expected_log: str +) -> None: """Test invalid MapData.add_trace_points.""" map_data = MapData() - with pytest.raises(ValueError, match=expected_error): + with pytest.raises(ValueError, match=expected_error), LogCapture() as log: map_data.add_trace_points(value) + log.check_present( + ( + "deebot_client.map", + "ERROR", + expected_log, + ) + ) @pytest.mark.parametrize( - ("index", "base64_data", "expected_error"), + ("index", "base64_data", "expected_error", "expected_log"), [ ( 10000, "invalid_base64", - "Index out of bounds;index:10000,base64_data:invalid_base64", + "Index out of bounds", + "Index out of bounds; index:10000, base64_data:invalid_base64", ), ( 1, "invalid_base64", - "Invalid symbol 95, offset 7.;index:1,base64_data:invalid_base64", + "Invalid symbol 95, offset 7.", + "Failed to update map piece: Invalid symbol 95, offset 7.; index:1, base64_data:invalid_base64", + ), + ( + 1, + "", + "Invalid 7z compressed data", + "Failed to update map piece: Invalid 7z compressed data; index:1, base64_data:", ), - (1, "", "Invalid 7z compressed data;index:1,base64_data:"), ], ) def test_MapData_update_map_piece_invalid( - index: int, base64_data: str, expected_error: str + index: int, base64_data: str, expected_error: str, expected_log: str ) -> None: """Test invalid MapData.update_map_piece.""" map_data = MapData() - with pytest.raises(ValueError, match=expected_error): + with pytest.raises(ValueError, match=expected_error), LogCapture() as log: map_data.update_map_piece(index, base64_data) + log.check_present( + ( + "deebot_client.map", + "ERROR", + expected_log, + ) + ) From bb22b73eb17d497e60d99bb6eeb88db25011c3c7 Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Fri, 23 May 2025 09:38:18 +0200 Subject: [PATCH 6/9] Implement code review suggestion --- deebot_client/map.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/deebot_client/map.py b/deebot_client/map.py index 36b9f816a..b83b58f6f 100644 --- a/deebot_client/map.py +++ b/deebot_client/map.py @@ -138,8 +138,8 @@ async def on_map_trace(event: MapTraceEvent) -> None: if event.start == 0: self._map_data.clear_trace_points() - if event.data.strip(): - self._map_data.add_trace_points(event.data) + if (data := event.data.strip()): + self._map_data.add_trace_points(data) unsubscribers.append(self._event_bus.subscribe(MapTraceEvent, on_map_trace)) From 82ad0d09f367b3ecd0e8473d8d4ef2e33797182b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 23 May 2025 07:38:54 +0000 Subject: [PATCH 7/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- deebot_client/map.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deebot_client/map.py b/deebot_client/map.py index b83b58f6f..17dc603fe 100644 --- a/deebot_client/map.py +++ b/deebot_client/map.py @@ -138,7 +138,7 @@ async def on_map_trace(event: MapTraceEvent) -> None: if event.start == 0: self._map_data.clear_trace_points() - if (data := event.data.strip()): + if data := event.data.strip(): self._map_data.add_trace_points(data) unsubscribers.append(self._event_bus.subscribe(MapTraceEvent, on_map_trace)) From 9a349d35d5185a62546332d9915a3710747e895c Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Fri, 23 May 2025 09:40:20 +0200 Subject: [PATCH 8/9] Update src/map.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/map.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/map.rs b/src/map.rs index 46be5b9a9..e18263bb9 100644 --- a/src/map.rs +++ b/src/map.rs @@ -368,9 +368,13 @@ impl MapData { &mut self, index: usize, crc32: u32, + base64_data: &str, ) -> Result { if index >= self.map_pieces.len() { - error!("Index out of bounds; index:{}", index); + error!( + "Index out of bounds; index:{}, base64_data:{}", + index, base64_data + ); return Err(PyValueError::new_err("Index out of bounds")); } Ok(self.map_pieces[index].crc32_indicates_update(crc32)) From 9a4bb5fe1e93c4985b7729e0c9948b7d577f20f3 Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Fri, 23 May 2025 07:49:44 +0000 Subject: [PATCH 9/9] Fix invalid codepilot suggestion --- src/map.rs | 6 +----- tests/rs/test_map.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/map.rs b/src/map.rs index e18263bb9..0601b64fb 100644 --- a/src/map.rs +++ b/src/map.rs @@ -368,13 +368,9 @@ impl MapData { &mut self, index: usize, crc32: u32, - base64_data: &str, ) -> Result { if index >= self.map_pieces.len() { - error!( - "Index out of bounds; index:{}, base64_data:{}", - index, base64_data - ); + error!("Index out of bounds; index:{}, crc32:{}", index, crc32); return Err(PyValueError::new_err("Index out of bounds")); } Ok(self.map_pieces[index].crc32_indicates_update(crc32)) diff --git a/tests/rs/test_map.py b/tests/rs/test_map.py index a41230d37..23edfbcb9 100644 --- a/tests/rs/test_map.py +++ b/tests/rs/test_map.py @@ -76,3 +76,17 @@ def test_MapData_update_map_piece_invalid( expected_log, ) ) + + +def test_MapData_map_piece_crc32_indicates_update_invalid() -> None: + """Test invalid MapData.map_piece_crc32_indicates_update.""" + map_data = MapData() + with pytest.raises(ValueError, match="Index out of bounds"), LogCapture() as log: + map_data.map_piece_crc32_indicates_update(1000, 1) + log.check_present( + ( + "deebot_client.map", + "ERROR", + "Index out of bounds; index:1000, crc32:1", + ) + )