solution of issue #9 #124
solution of issue #9 #124
Conversation
|
Hi, Thanks a lot for the PR. I get back to you on Friday and Saturday with feedback or approval as I need to test it with a mongo instance. |
|
Hi @VigneshVSV, No problem at all - just let me know once you’ve had a chance to test it. |
| from sqlalchemy import Integer, String, JSON, LargeBinary | ||
| from sqlalchemy.orm import Mapped, mapped_column, DeclarativeBase, MappedAsDataclass | ||
| from sqlite3 import DatabaseError | ||
| from pymongo import MongoClient, errors as mongo_errors |
There was a problem hiding this comment.
Can you add pymongo into the dependencies in pyproject.toml as one of the core dependencies? I will refactor it later into an optional dependency. But when we merge, it can still be published without installation errors.
| from pymongo import MongoClient, errors as mongo_errors | ||
| from ..param import Parameterized | ||
| from ..core.property import Property | ||
| class MongoThingDB: |
There was a problem hiding this comment.
Code formatting could be improved here, I would prefer if this class goes downwards towards the end of the file and there are spaces between imports and code definitions.
| """ | ||
| name = property if isinstance(property, str) else property.name | ||
| import base64, pickle | ||
| serialized_value = base64.b64encode(pickle.dumps(value)).decode("utf-8") |
There was a problem hiding this comment.
The serializer can be defined as
serializer = Serializers.for_object(self.id, self.thing_instance.__class__.__name__, name)
serialized_value = serializer.dumps(value)You can see this code in ThingDB below.
In this way, we dont have to always use pickle, because pickle usage must be inherently discouraged as its not a safe serializer due to possibility of arbitrary code execution. This may not true for the specific use case here, but must be followed as a general practice.
The Serializers is a Singleton for the specific python process & contains all the content types for all the properties (actions and events), defaulting to JSON. So once you specify the thing ID and thing class (as a string), it will know what serializer to use. This is not documented yet except the API reference, so sorry about that.
The base64 import can go to the top of file with all other imports.
| (see ZMQ messaging contract) from server side and a exception will be raised on the client. | ||
| """ | ||
|
|
||
| def __init__(self) -> None: |
There was a problem hiding this comment.
Can we undo this code deletion? This is supposed to be the Abstract Base Class for a Serializer. (May be we promote it collections.abc later, but until then we should have the definitions)
The type definitions picked up by the code editor for dumps and loads are also not working in many places.
| prepare_object_FSM(self) | ||
| prepare_object_storage(self, **kwargs) # use_default_db, db_config_file, use_json_file, json_filename | ||
| # Explicit auto-setup for default DB | ||
| if use_default_db: |
There was a problem hiding this comment.
One could leave this as it was, because kwargs already takes care of passing use_default_db into the prepare_object_storage function. I dont think there is a need of doing some preprocessing here.
|
|
||
| class TestProperty(TestCase): | ||
| @classmethod | ||
| def setUpClass(cls): |
There was a problem hiding this comment.
The tests are working as expected. Thank you for this.
Currently I realised I cannot integrate it in the CI/CD because of a lacking mongo instance. I have to think a little bit about how to set this up in a right way in github actions with long terms goals in mind.
So these tests can be placed in a file tests/working - yet to be integrated/test_07_properties_mongodb.py (not the not working folder). It can be named class TestMongoDBOperations(TestCase)
In the current state, there is also a bug that the setUpClass is defined twice, probably left out by an AI agent, so if I trigger the tests pipeline, it will fail anway.
Once you push all the requested changes, we can run the pipeline (which will most likely pass) abd merge it.
Thanks for the work again.
|
Hi @katerinaonusk , My review is complete. Just some minor cosmetic changes and I think it should be good to go. |
|
Hi @VigneshVSV, Thanks a lot for the detailed review!
I’ll try to complete these changes soon so we can run the pipeline and move forward with the merge. |
|
@katerinaonusk , feel free to take your time. I have to also add a docker compose file in another project with mongo in it. So it should take a few days for all of this to come together. Also, put a star if you like the project and tell your IoT, electronics & web developer friends about the project. |
2 participants
Adds mongo DB support for property persistence for device restarts. Solves issue #9.