Feature/add entity for topics #64
Conversation
WalkthroughThis PR introduces a new Entity domain model with complete layers: database migrations creating the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Layer
participant Service as EntityService
participant Repo as PostgreSQL<br/>Repository
participant DB as PostgreSQL
Note over Client,DB: Create Entity Flow
Client->>API: POST /entities/new<br/>(Bearer Token + Payload)
API->>API: Verify JWT & Role<br/>(non-Student required)
API->>API: Serialize entity_data<br/>to JSON string
API->>Service: create_entity(topic_id, type,<br/>order_id, version, data)
Service->>Service: Build UpsertEntityDto
Service->>Repo: create(UpsertEntityDto)
Repo->>DB: INSERT INTO entities<br/>RETURNING *
DB-->>Repo: Entity (with generated UUID)
Repo-->>Service: Entity
Service-->>API: Entity
API-->>Client: 201 Created + Entity
Note over Client,DB: Retrieve Entities by Topic
Client->>API: GET /topics/{id}/entities<br/>(Bearer Token)
API->>API: Verify JWT
API->>Service: get_by_topic_id(topic_id)
Service->>Repo: get_by_topic_id(topic_id)
Repo->>DB: SELECT FROM entities<br/>WHERE topic_id = ?<br/>ORDER BY order_id ASC
DB-->>Repo: Vec<Entity>
Repo-->>Service: Vec<Entity>
Service-->>API: Vec<Entity>
API->>API: Wrap in GetEntitiesForTopicResponseDto
API-->>Client: 200 OK + Response DTO
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
backend/migrations/20251007172446_created_entity_table.up.sql (1)
9-9: Consider JSONB instead of TEXT forentity_data.If
entity_datastores structured data (e.g., JSON), usingJSONBenables indexing, querying, and validation. TEXT lacks these capabilities and may lead to inefficient queries.If the data is JSON, apply this diff:
entity_version INTEGER NOT NULL DEFAULT 1, - entity_data TEXT NOT NULL + entity_data JSONB NOT NULL );If data format varies by type, consider type-specific validation at the application layer.
backend/migrations/20251007172446_created_entity_table.down.sql (1)
3-3: Remove duplicate comment.The comment is unnecessarily repeated from line 1.
Apply this diff:
DROP TABLE entities; --- Add down migration script here DROP TYPE ENTITY_TYPE;backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (2)
12-38: Minor: Redundant type cast on line 29.The cast
entity.r#type as EntityTypeis redundant sinceentity.r#typeis already of typeEntityType. However, this is harmless and may aid explicitness for sqlx macro type inference.Otherwise, the create method is correctly implemented:
- UUID v4 generation for new entities
- Proper parameter binding
- Standard error propagation
40-73: Minor: Redundant type cast on line 64.Similar to the create method, the cast
entity.r#type as EntityTypeon line 64 is redundant.Otherwise, the update method is correctly implemented:
- UPDATE with RETURNING clause to return the updated entity
- Proper type mapping with
AS "type: EntityType"for sqlx- fetch_one is appropriate for single-row updates
backend/src/api/topics/routes.rs (1)
15-15: Minor: Typo in imported DTO name.The DTO name
GetEntitiesForTopicResponceDtocontains a typo: "Responce" should be "Response". This appears to be defined in the DTO module (backend/src/dto/entity/mod.rs) and is used consistently, but consider fixing the typo in the DTO definition and updating all usages.backend/src/domain/entity/model.rs (2)
16-22: Questionable: Ordering traits may not have meaningful semantics.Deriving
OrdandPartialOrdforEntityDataestablishes an ordering between different entity types (e.g., Text vs Video), but it's unclear what semantic meaning this ordering has. Enum variant ordering in Rust is based on declaration order, which may not represent any business logic.Consider whether ordering is actually needed for
EntityData. If not, remove theOrdandPartialOrdderives:-#[derive(Serialize, Deserialize, ToSchema, Eq, PartialEq, Ord, PartialOrd, Clone, Hash)] +#[derive(Serialize, Deserialize, ToSchema, Eq, PartialEq, Clone, Hash)] #[serde(rename_all = "lowercase")] pub enum EntityData {
37-38: Minor: Remove commented-out code.The commented-out
entity_idfield and explanation should be removed. Comments explaining why something is not present are typically unnecessary—the absence speaks for itself. If the rationale is important for future maintainers, consider adding a brief module-level doc comment instead.pub r#type: EntityType, - // this is not a meta-table, so we don't need to have a foreign key to external model - //pub entity_id: Uuid, pub order_id: i32,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/migrations/20251007172446_created_entity_table.down.sql(1 hunks)backend/migrations/20251007172446_created_entity_table.up.sql(1 hunks)backend/src/api/entity/mod.rs(1 hunks)backend/src/api/entity/routes.rs(1 hunks)backend/src/api/mod.rs(1 hunks)backend/src/api/topics/mod.rs(2 hunks)backend/src/api/topics/routes.rs(2 hunks)backend/src/app.rs(3 hunks)backend/src/domain/entity/mod.rs(1 hunks)backend/src/domain/entity/model.rs(1 hunks)backend/src/domain/entity/repository.rs(1 hunks)backend/src/domain/entity/service.rs(1 hunks)backend/src/domain/mod.rs(1 hunks)backend/src/dto/entity/mod.rs(1 hunks)backend/src/dto/mod.rs(1 hunks)backend/src/gen_openapi.rs(3 hunks)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs(1 hunks)backend/src/infrastructure/db/postgres/repositories/mod.rs(1 hunks)backend/src/main.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
backend/src/api/entity/routes.rs (3)
frontend/src/api/account.ts (1)
UserRole(5-5)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
create(12-38)backend/src/domain/entity/repository.rs (1)
create(13-13)
backend/src/domain/entity/service.rs (2)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
get_by_topic_id(85-108)backend/src/domain/entity/repository.rs (1)
get_by_topic_id(16-16)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (3)
backend/src/api/entity/routes.rs (1)
create(26-49)backend/src/domain/entity/repository.rs (4)
create(13-13)update(14-14)delete(15-15)get_by_topic_id(16-16)backend/src/domain/entity/service.rs (1)
get_by_topic_id(34-36)
backend/src/api/topics/routes.rs (2)
backend/src/api/exam/routes.rs (1)
get_entities(201-245)backend/src/domain/exam/service.rs (1)
get_entities(69-71)
backend/src/api/topics/mod.rs (2)
backend/src/api/entity/mod.rs (1)
configure(17-22)backend/src/api/topics/routes.rs (1)
get_entities(174-182)
backend/src/domain/entity/repository.rs (3)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (4)
create(12-38)update(40-73)delete(75-83)get_by_topic_id(85-108)backend/src/api/entity/routes.rs (1)
create(26-49)backend/src/domain/entity/service.rs (1)
get_by_topic_id(34-36)
backend/src/main.rs (1)
backend/src/domain/entity/service.rs (1)
new(12-14)
backend/src/api/entity/mod.rs (2)
backend/src/api/topics/mod.rs (1)
configure(25-48)backend/src/api/entity/routes.rs (1)
create(26-49)
backend/src/app.rs (2)
backend/src/api/entity/mod.rs (1)
configure(17-22)backend/src/api/topics/mod.rs (1)
configure(25-48)
backend/src/gen_openapi.rs (2)
backend/src/domain/entity/service.rs (1)
new(12-14)backend/src/domain/video/service.rs (1)
new(38-53)
🔇 Additional comments (34)
backend/migrations/20251007172446_created_entity_table.up.sql (1)
7-7: Consider uniqueness constraint on(topic_id, order_id).Without a unique constraint, multiple entities can have the same
order_idwithin a topic, potentially causing ordering conflicts or duplicate entries.Verify if duplicate
order_idvalues per topic are intentional. If ordering should be unique per topic, apply this diff:entity_version INTEGER NOT NULL DEFAULT 1, entity_data TEXT NOT NULL + CONSTRAINT entities_topic_order_unique UNIQUE (topic_id, order_id) );backend/src/domain/mod.rs (1)
10-11: LGTM!The
entitymodule is properly declared and follows the existing module ordering pattern.backend/src/dto/mod.rs (1)
5-5: LGTM!The
entityDTO module is properly declared and maintains alphabetical ordering.backend/src/infrastructure/db/postgres/repositories/mod.rs (1)
4-4: LGTM!The
entity_repomodule is properly declared and follows alphabetical ordering.backend/src/api/mod.rs (1)
5-5: LGTM!The
entityAPI module is properly declared and maintains alphabetical ordering.backend/src/api/topics/mod.rs (4)
10-13: LGTM!The
EntityServiceimport is correctly added and follows the existing import pattern.
17-23: LGTM!The
entity_servicefield is properly added toTopicsStateand maintains consistency with other service fields.
25-36: LGTM!The
entity_serviceparameter is correctly threaded through theconfigurefunction and properly initialized in the state.
38-47: LGTM!The
get_entitiesroute is properly registered using theroutes!macro pattern consistent with other route registrations.backend/src/domain/entity/mod.rs (1)
1-3: LGTM!The entity domain module structure properly separates model, repository, and service concerns, consistent with other domain modules in the codebase.
backend/src/app.rs (3)
8-9: LGTM!The EntityService import is correctly placed and consistent with other service imports in the domain module.
30-30: LGTM!The entity field is correctly added to the Services struct with appropriate visibility and type.
95-97: LGTM!The router nesting correctly integrates the EntityService:
- Topics router receives a clone of the entity service (line 95)
- Entities router receives the moved entity service (line 97, final use)
The ownership strategy is appropriate.
backend/src/gen_openapi.rs (4)
8-9: LGTM!The EntityService import is correctly placed and consistent with other domain service imports.
48-49: LGTM!The addition of
.clone()to the third argument ofVideoService::newis correct, asdummyneeds to remain available for the subsequentEntityService::new(dummy)call on line 50.
50-50: LGTM!The EntityService instantiation correctly consumes the dummy repository on its final use.
62-62: LGTM!The entity field is correctly added to the Services struct for OpenAPI generation.
backend/src/api/entity/mod.rs (2)
1-16: LGTM!The EntityState struct is correctly designed with:
- Appropriate derives (Clone, FromRef) for axum state extraction
- JWT and EntityService fields properly typed
- Module imports are complete and consistent with similar API modules
17-22: LGTM!The configure function correctly:
- Accepts jwt and service parameters matching the call site in app.rs
- Constructs and wires the EntityState
- Registers the create route using the routes! macro
- Returns an OpenApiRouter for integration
backend/src/domain/entity/service.rs (4)
1-10: LGTM!The EntityService struct and imports are correctly structured:
- Follows the established service pattern in the codebase
- Properly imports domain models, repository trait, and DTOs
- Clone derive enables service sharing across handlers
11-14: LGTM!The constructor follows the standard pattern used throughout the codebase for service initialization.
16-33: LGTM!The create_entity method correctly:
- Constructs UpsertEntityDto with id: None (repository generates UUID)
- Maps all provided parameters to DTO fields
- Delegates to the repository layer
- Returns Result<()> for error propagation
34-36: LGTM!The get_by_topic_id method correctly delegates to the repository and returns the appropriate type.
backend/src/main.rs (3)
18-19: LGTM!The EntityService import is correctly placed with other domain service imports.
109-109: LGTM!The EntityService is correctly instantiated with a clone of the PostgreSQL repository, following the pattern used for other services.
120-120: LGTM!The entity field is correctly added to the Services struct initialization in the bootstrap sequence.
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (3)
1-9: LGTM!All necessary imports are present for the repository implementation.
75-83: LGTM!The delete method is correctly implemented with a straightforward DELETE query by id.
85-109: LGTM!The get_by_topic_id method is correctly implemented:
- Filters by topic_id parameter
- Orders results by order_id for consistent ordering
- Proper type mapping for EntityType
- fetch_all is appropriate for returning multiple entities
backend/src/api/entity/routes.rs (4)
1-10: LGTM!All necessary imports are present for the create handler implementation.
11-25: LGTM!The OpenAPI documentation is complete with:
- Appropriate HTTP method and path
- Correct request/response types
- Comprehensive status code coverage
- Security requirement specified
31-33: Verify access control for Teacher and Admin roles.The current check denies creation for Students but allows it for Teachers and Admins. Please confirm this is the intended behavior.
37-49: LGTM!The service invocation and response construction are correctly implemented:
- All parameters are passed to create_entity
- Error propagation via ? operator
- Success response returns the expected DTO
backend/src/api/topics/routes.rs (1)
174-182: Verify that entities should be accessible to all authenticated users.The handler ignores the
claimsparameter (prefixed with_) and does not perform any access control checks, unlike theget_examshandler above which usesclaims.subandclaims.role.Please confirm whether entities should be readable by all authenticated users (Students, Teachers, Admins) or if access should be restricted based on role or user-topic relationship.
| @@ -0,0 +1,4 @@ | |||
| -- Add down migration script here | |||
| DROP TABLE entities | |||
There was a problem hiding this comment.
Syntax error: missing semicolon.
The DROP TABLE statement is missing a semicolon, which will cause the down migration to fail.
Apply this diff:
-- Add down migration script here
-DROP TABLE entities
+DROP TABLE entities;
-- Add down migration script here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DROP TABLE entities | |
| -- Add down migration script here | |
| DROP TABLE entities; | |
| -- Add down migration script here |
🤖 Prompt for AI Agents
In backend/migrations/20251007172446_created_entity_table.down.sql around line
2, the DROP TABLE statement is missing a trailing semicolon which will cause the
down migration to fail; fix by appending a semicolon to the end of the DROP
TABLE entities statement (i.e., change it to end with ";") so the SQL statement
is properly terminated.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| #[derive(Serialize, Deserialize, sqlx::Type, ToSchema, Debug)] | ||
| #[sqlx(type_name = "ENTITY_TYPE")] | ||
| #[sqlx(rename_all = "lowercase")] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum EntityType { | ||
| Exam, | ||
| Text, | ||
| Video, // here we can add more entity types in the future | ||
| } |
There was a problem hiding this comment.
Critical: EntityType and EntityData variant mismatch.
The EntityType enum includes an Exam variant, but the corresponding EntityData enum (lines 16-22) does not have a matching variant. This inconsistency will cause runtime errors when trying to create or deserialize an entity with type Exam.
The TODO comment on line 19 suggests exams will be migrated in the future, but having an Exam variant in EntityType without corresponding data structure creates a partially implemented feature that can break at runtime.
Consider one of these approaches:
Option 1: Remove Exam variant until implementation is ready
pub enum EntityType {
- Exam,
Text,
Video,
}Option 2: Add Exam variant to EntityData
pub enum EntityData {
+ Exam { /* exam data fields */ },
Text { data: String },
Video { url: String, transcript: String },
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/src/domain/entity/model.rs around lines 6-14, the EntityType enum
contains an Exam variant but EntityData lacks a matching Exam variant, creating
a mismatch that will break serialization/deserialization; fix by either (a)
removing the Exam variant from EntityType until the Exam data structure is
implemented, or (b) adding a corresponding Exam variant to the EntityData enum
(with appropriate fields), updating serde/sqlx attributes if necessary, and
adding any required tests/migrations to ensure Exam entities
serialize/deserialize and round‑trip through the DB correctly.
| impl From<String> for EntityData { | ||
| fn from(s: String) -> Self { | ||
| let data: EntityData = | ||
| serde_json::from_str(s.as_str()).expect("cannot deserialize entity data"); | ||
| data | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Panic on invalid JSON input.
The From<String> implementation uses .expect(), which will panic if the input string is not valid JSON or doesn't match the EntityData schema. Since this is used when deserializing from the database (as seen in the repository implementation), corrupted or invalid database data will crash the application.
Replace the From trait with TryFrom to properly handle errors:
-impl From<String> for EntityData {
- fn from(s: String) -> Self {
- let data: EntityData =
- serde_json::from_str(s.as_str()).expect("cannot deserialize entity data");
- data
- }
+impl TryFrom<String> for EntityData {
+ type Error = serde_json::Error;
+
+ fn try_from(s: String) -> Result<Self, Self::Error> {
+ serde_json::from_str(&s)
+ }
}This will require updating the sqlx integration to handle the Result type, but prevents panics from invalid data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl From<String> for EntityData { | |
| fn from(s: String) -> Self { | |
| let data: EntityData = | |
| serde_json::from_str(s.as_str()).expect("cannot deserialize entity data"); | |
| data | |
| } | |
| } | |
| impl TryFrom<String> for EntityData { | |
| type Error = serde_json::Error; | |
| fn try_from(s: String) -> Result<Self, Self::Error> { | |
| serde_json::from_str(&s) | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/src/domain/entity/model.rs around lines 24-30, the From<String> impl
currently uses expect() which will panic on invalid JSON; replace it by
implementing TryFrom<String> for EntityData that returns Result<EntityData,
serde_json::Error> (or a suitable error type) and deserialize with
serde_json::from_str returning the Err instead of panicking; then update all
call sites and the sqlx integration to accept and propagate the Result (e.g.,
map the database value through EntityData::try_from or propagate the error in
query mapping) so invalid/corrupted JSON is handled gracefully rather than
crashing the process.
| #[impl_unimplemented(DummyRepository)] | ||
| #[async_trait] | ||
| pub trait EntityRepository { | ||
| async fn create(&self, entity: UpsertEntityDto) -> Result<()>; | ||
| async fn update(&self, id: Uuid, entity: UpsertEntityDto) -> Result<Entity>; | ||
| async fn delete(&self, id: Uuid) -> Result<()>; | ||
| async fn get_by_topic_id(&self, topic_id: i32) -> Result<Vec<Entity>>; | ||
| } |
There was a problem hiding this comment.
Major: Inconsistent return type between create and update.
The create method returns Result<()>, making it impossible for callers to retrieve the generated UUID of the newly created entity. In contrast, the update method returns Result<Entity>. This inconsistency forces clients to either:
- Provide their own UUID (security/design concern)
- Query back the created entity using another field (inefficient)
- Not have access to the created entity's ID
Looking at the implementation in entity_repo.rs (lines 11-37), a UUID is generated server-side but never returned.
Update the create signature to return the created entity:
#[impl_unimplemented(DummyRepository)]
#[async_trait]
pub trait EntityRepository {
- async fn create(&self, entity: UpsertEntityDto) -> Result<()>;
+ async fn create(&self, entity: UpsertEntityDto) -> Result<Entity>;
async fn update(&self, id: Uuid, entity: UpsertEntityDto) -> Result<Entity>;
async fn delete(&self, id: Uuid) -> Result<()>;
async fn get_by_topic_id(&self, topic_id: i32) -> Result<Vec<Entity>>;
}Then update the repository implementation to use RETURNING * clause in the INSERT statement (similar to the UPDATE query).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[impl_unimplemented(DummyRepository)] | |
| #[async_trait] | |
| pub trait EntityRepository { | |
| async fn create(&self, entity: UpsertEntityDto) -> Result<()>; | |
| async fn update(&self, id: Uuid, entity: UpsertEntityDto) -> Result<Entity>; | |
| async fn delete(&self, id: Uuid) -> Result<()>; | |
| async fn get_by_topic_id(&self, topic_id: i32) -> Result<Vec<Entity>>; | |
| } | |
| #[impl_unimplemented(DummyRepository)] | |
| #[async_trait] | |
| pub trait EntityRepository { | |
| async fn create(&self, entity: UpsertEntityDto) -> Result<Entity>; | |
| async fn update(&self, id: Uuid, entity: UpsertEntityDto) -> Result<Entity>; | |
| async fn delete(&self, id: Uuid) -> Result<()>; | |
| async fn get_by_topic_id(&self, topic_id: i32) -> Result<Vec<Entity>>; | |
| } |
🤖 Prompt for AI Agents
In backend/src/domain/entity/repository.rs around lines 10-17, the create method
currently returns Result<()> which prevents returning the generated UUID; change
its signature to async fn create(&self, entity: UpsertEntityDto) ->
Result<Entity> so it returns the created Entity (matching update). In the
repository implementation file backend/src/domain/entity/entity_repo.rs (around
lines 11-37) modify the INSERT to use a RETURNING * clause and map the returned
row to an Entity to return (including the generated UUID), and update any trait
impls/tests to reflect the new create return type.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
backend/src/dto/entity/mod.rs
Outdated
| #[derive(Serialize)] | ||
| pub struct GetEntitiesForTopicResponceDto { | ||
| pub entities: Vec<Entity>, | ||
| } |
There was a problem hiding this comment.
Minor: Typo in struct name.
The struct name GetEntitiesForTopicResponceDto has a typo: "Responce" should be "Response".
-#[derive(Serialize)]
-pub struct GetEntitiesForTopicResponceDto {
+#[derive(Serialize)]
+pub struct GetEntitiesForTopicResponseDto {
pub entities: Vec<Entity>,
}Also update all references to this struct in other files (e.g., routes).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Serialize)] | |
| pub struct GetEntitiesForTopicResponceDto { | |
| pub entities: Vec<Entity>, | |
| } | |
| #[derive(Serialize)] | |
| pub struct GetEntitiesForTopicResponseDto { | |
| pub entities: Vec<Entity>, | |
| } |
🤖 Prompt for AI Agents
In backend/src/dto/entity/mod.rs around lines 35 to 38, the struct name
GetEntitiesForTopicResponceDto contains a typo ("Responce"); rename it to
GetEntitiesForTopicResponseDto and update the derive/visibility line
accordingly, then search and replace all references/imports/usages of the old
name across the codebase (routes, tests, modules, and mod declarations) to the
new name and run cargo build/test to ensure no remaining references.
Major: Missing ToSchema derive for OpenAPI generation.
GetEntitiesForTopicResponceDto is used as a response DTO but lacks the ToSchema derive, preventing proper OpenAPI documentation generation for the endpoint that returns it.
-#[derive(Serialize)]
-pub struct GetEntitiesForTopicResponceDto {
+#[derive(Serialize, ToSchema)]
+pub struct GetEntitiesForTopicResponceDto {
pub entities: Vec<Entity>,
}Note: This also requires the Entity struct to have ToSchema (flagged in separate comment on model.rs).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Serialize)] | |
| pub struct GetEntitiesForTopicResponceDto { | |
| pub entities: Vec<Entity>, | |
| } | |
| #[derive(Serialize, ToSchema)] | |
| pub struct GetEntitiesForTopicResponceDto { | |
| pub entities: Vec<Entity>, | |
| } |
🤖 Prompt for AI Agents
In backend/src/dto/entity/mod.rs around lines 35 to 38, the
GetEntitiesForTopicResponceDto struct is missing the ToSchema derive required
for OpenAPI generation; add the ToSchema derive to the struct (e.g.,
#[derive(Serialize, ToSchema)]) and ensure you import ToSchema (use
utoipa::ToSchema) at the top of the file, and also make sure the Entity type
derives ToSchema in backend/src/model.rs as flagged so the endpoint schema can
be generated correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/dto/entity/mod.rs (1)
34-37: Fix the typo in struct name.The struct name still contains the typo "Responce" instead of "Response". This was flagged in a previous review but remains unfixed. The typo will propagate to any code that references this DTO.
Apply this diff to fix the typo:
-#[derive(Serialize, ToSchema)] -pub struct GetEntitiesForTopicResponceDto { +#[derive(Serialize, ToSchema)] +pub struct GetEntitiesForTopicResponseDto { pub entities: Vec<Entity>, }Note: You'll also need to update all references to this struct throughout the codebase (imports, usage in route handlers, etc.).
🧹 Nitpick comments (1)
backend/src/api/entity/routes.rs (1)
34-35: Use a more professional error variant name.The error handling has been improved from the previous
.expect()usage, butLMSError::ShitHappenedis an unprofessional error variant name for production code. The previous review suggestedLMSError::InternalServerError, which would be more appropriate.Consider using a more professional error variant:
let entity_data: String = serde_json::to_string(&payload.entity_data) - .map_err(|e| LMSError::ShitHappened(format!("Failed to serialize entity data: {}", e)))?; + .map_err(|e| LMSError::InternalServerError(format!("Failed to serialize entity data: {}", e)))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/api/entity/routes.rs(1 hunks)backend/src/domain/entity/model.rs(1 hunks)backend/src/dto/entity/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/domain/entity/model.rs
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/api/entity/routes.rs (2)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
create(12-38)backend/src/domain/entity/repository.rs (1)
create(13-13)
cfrt-dev
left a comment
cfrt-dev
left a comment
There was a problem hiding this comment.
В целом все ок, поправь мелкие недочеты
| }; | ||
| #[utoipa::path( |
There was a problem hiding this comment.
nitpick: добавляй тут пустые строчки
backend/src/dto/entity/mod.rs
Outdated
| pub entity_version: i32, | ||
| pub entity_data: String, | ||
| } | ||
| #[derive(ToSchema, Serialize)] |
There was a problem hiding this comment.
Нужно ли тут отсылать json? Мне кажется просто 201 пойдет
There was a problem hiding this comment.
Добавляй пустые строчки между логическими блоками, пж пж
| pub mod task; | ||
| pub mod topics; | ||
| pub mod video; | ||
|
|
| ) | ||
| VALUES ($1, $2, $3, $4, $5, $6) | ||
| "#, | ||
| Uuid::new_v4(), |
There was a problem hiding this comment.
у тебя бд автоматически генерирует uuid, можешь просто не инсертить его
| entity.entity_version, | ||
| entity.entity_data | ||
| ) | ||
| .fetch_one(&mut *conn) |
There was a problem hiding this comment.
as_mut
и в остальные места добавь
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/api/entity/routes.rs(1 hunks)backend/src/api/topics/routes.rs(2 hunks)backend/src/domain/entity/service.rs(1 hunks)backend/src/dto/entity/mod.rs(1 hunks)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/infrastructure/db/postgres/repositories/entity_repo.rs
- backend/src/dto/entity/mod.rs
🧰 Additional context used
🧬 Code graph analysis (2)
backend/src/domain/entity/service.rs (2)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
get_by_topic_id(83-106)backend/src/domain/entity/repository.rs (1)
get_by_topic_id(16-16)
backend/src/api/entity/routes.rs (2)
backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
create(12-37)backend/src/domain/entity/repository.rs (1)
create(13-13)
🔇 Additional comments (3)
backend/src/api/entity/routes.rs (1)
28-51: LGTM!The handler implementation is well-structured with proper access control, error handling, and service delegation.
backend/src/domain/entity/service.rs (1)
1-40: LGTM!The service layer is cleanly implemented with proper separation of concerns. Both methods correctly delegate to the repository layer, and the DTO construction in
create_entityis straightforward.backend/src/api/topics/routes.rs (1)
175-183: Verify authorization in get_entities handler
Theget_entitiesendpoint ignoresclaims(uses_claims) and callsentity_service.get_by_topic_id(topic_id)without user/role checks, whileget_examsenforces access viaclaims.subandclaims.role. Confirm whether all authenticated users may view all entities; if not, update this handler (and service) to accept and enforceclaims.sub/claims.roleas inget_exams.
| #[async_trait] | ||
| impl EntityRepository for RepositoryPostgres { | ||
| async fn create(&self, entity: UpsertEntityDto) -> Result<()> { | ||
| let mut tx = self.pool.begin().await?; |
There was a problem hiding this comment.
А зачем везде транзакцию использовать?
earhater
force-pushed
the
feature/add-entity-for-topics
branch
from
0ca1eee to
a49751e
Compare
|
wait for my review pls |
wrapped entites into Responce DTO cleaned up imports minor fixes based on AI code review Delete lecture_repo.rs fixed typo review fixes 200 -> 201 in /entity/new yet another fix ( changed pool.begin() to pool.acquire() and tx to conn) prepared cache
earhater
force-pushed
the
feature/add-entity-for-topics
branch
from
a49751e to
85d41bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/domain/entity/service.rs (1)
18-35: Consider adding a blank line for readability.The method logic is correct, but adding a blank line between the DTO construction (lines 26-33) and the repository call (line 34) would improve readability by separating logical blocks.
Apply this diff:
r#type, order_id, entity_version, entity_data, }; + self.repo.create(dto).await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/.sqlx/query-403f07092c87df8d899533b613353566bcacc6f92616b818d6de26a8fcae397d.json(1 hunks)backend/.sqlx/query-9c4f126eda95e6f773e20d7482dbbc04131283c40e6ab5bac34b6bf97a0b10ea.json(1 hunks)backend/.sqlx/query-d37690d85e84b3bc8f9cbe792d64068b26533f0c4082b3ebae1220b3d3131d79.json(1 hunks)backend/.sqlx/query-f753adb70258e80f403fa61db4c6fad5f1b8854ebfec6e62c08388e1f499e040.json(1 hunks)backend/migrations/20251007172446_created_entity_table.down.sql(1 hunks)backend/migrations/20251007172446_created_entity_table.up.sql(1 hunks)backend/src/api/entity/mod.rs(1 hunks)backend/src/api/entity/routes.rs(1 hunks)backend/src/api/mod.rs(1 hunks)backend/src/api/topics/mod.rs(2 hunks)backend/src/api/topics/routes.rs(2 hunks)backend/src/app.rs(3 hunks)backend/src/domain/entity/mod.rs(1 hunks)backend/src/domain/entity/model.rs(1 hunks)backend/src/domain/entity/repository.rs(1 hunks)backend/src/domain/entity/service.rs(1 hunks)backend/src/domain/mod.rs(1 hunks)backend/src/dto/entity/mod.rs(1 hunks)backend/src/dto/mod.rs(1 hunks)backend/src/gen_openapi.rs(3 hunks)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs(1 hunks)backend/src/infrastructure/db/postgres/repositories/mod.rs(1 hunks)backend/src/main.rs(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/.sqlx/query-9c4f126eda95e6f773e20d7482dbbc04131283c40e6ab5bac34b6bf97a0b10ea.json
- backend/.sqlx/query-403f07092c87df8d899533b613353566bcacc6f92616b818d6de26a8fcae397d.json
🚧 Files skipped from review as they are similar to previous changes (13)
- backend/src/domain/mod.rs
- backend/src/dto/mod.rs
- backend/src/api/entity/mod.rs
- backend/src/api/mod.rs
- backend/src/app.rs
- backend/migrations/20251007172446_created_entity_table.down.sql
- backend/migrations/20251007172446_created_entity_table.up.sql
- backend/src/api/topics/routes.rs
- backend/src/api/topics/mod.rs
- backend/src/infrastructure/db/postgres/repositories/entity_repo.rs
- backend/src/domain/entity/repository.rs
- backend/src/domain/entity/model.rs
- backend/src/gen_openapi.rs
🧰 Additional context used
🧬 Code graph analysis (3)
backend/src/main.rs (1)
backend/src/domain/entity/service.rs (1)
new(14-16)
backend/src/api/entity/routes.rs (2)
backend/src/domain/entity/repository.rs (1)
create(13-13)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
create(12-37)
backend/src/domain/entity/service.rs (2)
backend/src/domain/entity/repository.rs (1)
get_by_topic_id(16-16)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
get_by_topic_id(83-106)
🔇 Additional comments (14)
backend/src/domain/entity/mod.rs (1)
1-3: LGTM!Standard module organization following Rust conventions. The three submodules (model, repository, service) establish a clean domain layer structure.
backend/src/infrastructure/db/postgres/repositories/mod.rs (1)
4-4: LGTM!The new
entity_repomodule is properly integrated and maintains the alphabetical ordering of repository modules.backend/.sqlx/query-f753adb70258e80f403fa61db4c6fad5f1b8854ebfec6e62c08388e1f499e040.json (1)
1-79: Auto-generated SQLx metadata - no review needed.This file is machine-generated by SQLx's offline mode and should not be manually edited. It provides compile-time query verification for the entities UPDATE query.
backend/src/main.rs (3)
18-20: LGTM!EntityService is properly imported alongside other domain services, maintaining consistency with the existing service architecture.
109-109: LGTM!EntityService instantiation follows the established pattern, using
db_repo.clone()consistent with other repository-backed services.
120-120: LGTM!The
entityservice is correctly added to theServicesstruct, completing the integration of EntityService into the application's service layer.backend/src/api/entity/routes.rs (3)
1-11: LGTM!Import organization is clean and includes all necessary dependencies for the handler.
13-27: LGTM!OpenAPI documentation is accurate and complete. The status 201 response correctly matches the handler's return value.
28-51: LGTM!The handler implementation is solid:
- Access control properly restricts entity creation to non-Student roles
- JSON serialization uses proper error handling (previous
.expect()issue resolved)- Service delegation is clean and follows the established pattern
- Returns appropriate HTTP 201 Created status
backend/src/domain/entity/service.rs (2)
13-16: LGTM!Constructor follows the standard service pattern, accepting a repository trait object.
37-39: LGTM!The
get_by_topic_idmethod correctly delegates to the repository layer, maintaining a clean separation of concerns.backend/src/dto/entity/mod.rs (3)
11-19: LGTM!The
UpsertEntityDtois well-structured:
idis nowOption<Uuid>, correctly distinguishing between create (None) and update (Some) operations (previous issue resolved)- Proper derives for database interaction and serialization
entity_dataasStringaligns with JSON serialization in the handler layer
21-28: LGTM!The
UpsertEntityRequestDtois appropriately designed:
- Proper derives for API layer including
ToSchemafor OpenAPI documentation- No
idfield, allowing the server to generate UUIDs for new entities- Uses
EntityDatadomain type for structured input, which the handler serializes to JSON string
30-33: LGTM!The
GetEntitiesForTopicResponseDtois correct:
- Proper derives including
ToSchemafor OpenAPI documentation (previous issue resolved)- Struct name correctly spelled as "Response" (previous typo fixed)
- Clean wrapper for domain entities
JustMarfix
left a comment
JustMarfix
left a comment
There was a problem hiding this comment.
В целом норм, но:
- Поправь комменты Макса плиз.
- Не забывай локально запускать
cargo clippy - Перед каждым коммитом запускай у себя
cargo fmt --all - Посмотри на мои комменты и поправь.
| @@ -0,0 +1,4 @@ | |||
| -- Add down migration script here | |||
| DROP TABLE entities | |||
| #[derive(Serialize, Deserialize, ToSchema, Eq, PartialEq, Ord, PartialOrd, Clone, Hash)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum EntityData { | ||
| //TODO migrate exams in the future |
There was a problem hiding this comment.
Имеется желание сделать это в рамках этого PR.
| #[impl_unimplemented(DummyRepository)] | ||
| #[async_trait] | ||
| pub trait EntityRepository { | ||
| async fn create(&self, entity: UpsertEntityDto) -> Result<()>; | ||
| async fn update(&self, id: Uuid, entity: UpsertEntityDto) -> Result<Entity>; | ||
| async fn delete(&self, id: Uuid) -> Result<()>; | ||
| async fn get_by_topic_id(&self, topic_id: i32) -> Result<Vec<Entity>>; | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/src/api/entity/routes.rs (1)
1-10: Consider adding a blank line between import groups.For improved readability, separate the crate-internal imports (lines 1-8) from the external axum imports (lines 9-10) with a blank line.
Apply this diff:
use crate::{ domain::account::model::UserRole, errors::{LMSError, Result}, }; + use axum::http::StatusCode; use axum::{Json, extract::State};backend/src/domain/entity/service.rs (1)
18-35: Consider adding input validation.The
create_entitymethod accepts parameters without validation. Consider validating:
order_idshould be non-negativeentity_versionshould be positiveentity_datashould not be emptyExample validation:
pub async fn create_entity( &self, topic_id: i32, r#type: EntityType, order_id: i32, entity_version: i32, entity_data: String, ) -> Result<Entity> { if order_id < 0 { return Err(LMSError::BadRequest("order_id must be non-negative".to_string())); } if entity_version <= 0 { return Err(LMSError::BadRequest("entity_version must be positive".to_string())); } if entity_data.trim().is_empty() { return Err(LMSError::BadRequest("entity_data cannot be empty".to_string())); } let dto = UpsertEntityDto { id: None, topic_id, r#type, order_id, entity_version, entity_data, }; self.repo.create(dto).await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/.sqlx/query-ecc3bcf860f09bf1d69a6288bab8c3051372092089fd558934f0bcdc0e216a0b.json(1 hunks)backend/migrations/20251007172446_created_entity_table.up.sql(1 hunks)backend/src/api/entity/routes.rs(1 hunks)backend/src/domain/entity/model.rs(1 hunks)backend/src/domain/entity/repository.rs(1 hunks)backend/src/domain/entity/service.rs(1 hunks)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/domain/entity/model.rs
- backend/src/infrastructure/db/postgres/repositories/entity_repo.rs
- backend/src/domain/entity/repository.rs
- backend/migrations/20251007172446_created_entity_table.up.sql
🧰 Additional context used
🧬 Code graph analysis (2)
backend/src/api/entity/routes.rs (2)
backend/src/domain/entity/repository.rs (1)
create(13-13)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
create(12-45)
backend/src/domain/entity/service.rs (2)
backend/src/domain/entity/repository.rs (1)
get_by_topic_id(16-16)backend/src/infrastructure/db/postgres/repositories/entity_repo.rs (1)
get_by_topic_id(91-114)
🔇 Additional comments (3)
backend/.sqlx/query-ecc3bcf860f09bf1d69a6288bab8c3051372092089fd558934f0bcdc0e216a0b.json (1)
1-78: Auto-generated SQLx cache file; query structure looks correct.This is a machine-generated SQLx query descriptor. The query metadata correctly captures the INSERT statement with proper type mappings:
- Five input parameters with correct types (Int4 for IDs/versions, custom enum for EntityType, Text for JSON data)
- Six output columns (id as Uuid, topic_id/order_id/entity_version as Int4, type as EntityType enum, entity_data as Text)
- Type casting (
type AS "type: EntityType") aligns with Rust domain model expectations- All fields marked non-nullable, which is appropriate for this INSERT operation
- Hash provides cache validation
Note: This file is regenerated during
sqlx prepareor build time; avoid manual edits as they will be overwritten.Verify that this query metadata aligns with the actual
entitiestable schema defined in the database migrations (columns, types, and non-nullability constraints).backend/src/api/entity/routes.rs (1)
33-35: LGTM! Role-based access control is properly implemented.The role check correctly prevents Students from creating entities and returns an appropriate 403 Forbidden response.
backend/src/domain/entity/service.rs (1)
8-11: LGTM! Service struct is well-defined.The struct properly encapsulates the repository dependency with appropriate Clone derivation.
| let entity_data: String = serde_json::to_string(&payload.entity_data) | ||
| .map_err(|e| LMSError::ShitHappened(format!("Failed to serialize entity data: {e}")))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using a more professional error variant.
The error handling uses LMSError::ShitHappened, which is unprofessional for production code. Consider using a more appropriate error variant like LMSError::InternalServerError or introducing a dedicated serialization error variant.
Apply this diff:
let entity_data: String = serde_json::to_string(&payload.entity_data)
- .map_err(|e| LMSError::ShitHappened(format!("Failed to serialize entity data: {e}")))?;
+ .map_err(|e| LMSError::InternalServerError(format!("Failed to serialize entity data: {e}")))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let entity_data: String = serde_json::to_string(&payload.entity_data) | |
| .map_err(|e| LMSError::ShitHappened(format!("Failed to serialize entity data: {e}")))?; | |
| let entity_data: String = serde_json::to_string(&payload.entity_data) | |
| .map_err(|e| LMSError::InternalServerError(format!("Failed to serialize entity data: {e}")))?; |
🤖 Prompt for AI Agents
In backend/src/api/entity/routes.rs around lines 36 to 37, replace the
unprofessional LMSError::ShitHappened variant used for serialization failures
with a proper error variant (e.g., LMSError::InternalServerError or add a new
LMSError::SerializationError). Update the map_err call to return the chosen
variant with the same formatted message, and if you add a new variant, update
the LMSError enum and any conversion/handler code (and tests) that match on
error variants so the new/changed variant is handled consistently.
4 participants
Summary by CodeRabbit
Release Notes