-
Notifications
You must be signed in to change notification settings - Fork 0
1 polish the structure of the repo and add nextjs generated gui #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
1 polish the structure of the repo and add nextjs generated gui #2
Conversation
Pull Request Test Coverage Report for Build 20022565851Details
💛 - Coveralls |
JosePizarro3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. I will go through installing this locally, checking the README, and let you know how it goes.
I didn't review the frontend. Once I launch this I will take a look and give some comments.
JosePizarro3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some minor comments, I will rewrite the README later
| ) | ||
|
|
||
|
|
||
| class UpdateSchema(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model is typically used for partial updates (think HTTP PATCH): you want to allow the client to send only the fields that change, not the entire schema.
SchemaDefinition describes a full object (all required fields).
UpdateSchema describes a partial object where fields are optional so you can update only name, only version, or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah wait, then what needs to change to have optional fields in SchemaDefinition, so you can delete UpdateSchema. So this file content can be:
from datetime import datetime
from pydantic import BaseModel, Field
class SchemaDefinition(BaseModel):
id: str = Field(..., description="Unique identifier for the schema")
name: str | None = Field(
None ,
description="Human-readable name of the schema",
min_length=3,
)
version: str | None = Field(None, description="Version of the schema")
content: dict | None = Field(None, description="The actual schema content as a dictionary")
updated_at: datetime | None = Field(
None, description="Timestamp of the last update"
)as you see, I only defined one class SchemaDefinition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes applied
backend/main.py
Outdated
| schema_documents = list(schemas_collection.find()) | ||
|
|
||
| # Verbose loop: clarify what we're iterating over and why | ||
| for schema_document in schema_documents: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest, I am not sure what this logic does. You get the original_id from schema_document["_id"] and then you overwrite schema_document["_id"] with the original_id (which is already a string, shouldn't mutate to another type at any moment)
I think this method just needs to return list(schemas_collection.find())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can just take your suggestion -->> return list(schemas_collection.find())
BUT there could be issures with that: you shouldn’t return list(schemas_collection.find()) directly.
It will most likely raise TypeError: ObjectId is not JSON serializable because MongoDB’s _id field is a bson.ObjectId, not a string. FastAPI’s default JSON encoder doesn’t know how to serialize ObjectId.
Why it fails
Each document contains "_id": ObjectId(...).
When FastAPI tries to serialize the list, it encounters the ObjectId and crashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay; I have updated "get_all_schemas" in a different way. with: 1) jsonable_encoder with custom encoder
Best for: Typical REST APIs; teams using OpenAPI tooling; frontends that expect plain JSON strings for IDs.
Pros - One-liner conversion for _id and any nested ObjectIds.
- Plays nicely with FastAPI’s response models and OpenAPI schema.
- Keeps output stable (e.g., "_id": "656f...") and easy to consume.
Cons - You still need to define encoders for other BSON types if you use them (e.g., Decimal128, Binary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused now; why the ID needs to be of type bson? is there some benefits to this?
|
Nice |
JosePizarro3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an easier way of handling IDs, which is with hashlib. This way we will get rid of bson types.
I can implement that, but for you, perhaps is a good idea to simply assume that ìdis astr, and do not use ObjectIdfor any of thebackend/main.py` implementations.Once you get rid of these, I will take over and finish this pull request by implementing the handling of hashes for ids (so you can easily see what I mean).
So, in short:
- Go through my final comments
- Delete ObjectId usage and simply assume
idis a string you can safely use - Let me know by asking me to re-review
- I handled the id situation with hashes
- You take a look
- We merge
|
|
||
| # ---- Routes ---- | ||
| @app.get("/schemas") | ||
| async def get_all_schemas() -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruff fails because you should be using: list[dict[str, Any]] and no need to import typing classes like Dict, or List. Same goes for other app functions
|
|
||
| # Insert and obtain new ObjectId | ||
| result = schemas_collection.insert_one(schema.dict()) | ||
| inserted_oid = result.inserted_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm what are all these lines after insering the schema?
| version: str | ||
| content: dict # JSON schema | ||
| updated_at: datetime | ||
| id: str = Field(..., description="Unique identifier for the schema") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have another suggestion for ID: using hashlib instead and create a hash depending on the schema.dict() content.
this way we get rid off bson types
| if result.matched_count == 0: | ||
| raise HTTPException(status_code=404, detail="Schema not found") | ||
|
|
||
| # Optionally return the updated document (commented out by default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normally you should delete "dead code", without leaving it in the src code of your apps
means: delete these lines (from 104 to 109)
Description Message, I am creating a "Pull Request".