From 8e22ea05ffb9ed2b63f6928673f99fdc199c3932 Mon Sep 17 00:00:00 2001 From: filipriec Date: Fri, 20 Jun 2025 19:59:42 +0200 Subject: [PATCH] improvements and fixing of the tests --- .../handlers/post_table_definition.rs | 17 +++ .../post_table_definition_test2.rs | 2 +- .../post_table_definition_test3.rs | 10 +- .../post_table_definition_test4.rs | 100 ++++++++++++------ 4 files changed, 87 insertions(+), 42 deletions(-) diff --git a/server/src/table_definition/handlers/post_table_definition.rs b/server/src/table_definition/handlers/post_table_definition.rs index 26e1046..047e0a0 100644 --- a/server/src/table_definition/handlers/post_table_definition.rs +++ b/server/src/table_definition/handlers/post_table_definition.rs @@ -104,7 +104,21 @@ pub async fn post_table_definition( db_pool: &PgPool, request: PostTableDefinitionRequest, ) -> Result { + if request.profile_name.trim().is_empty() { + return Err(Status::invalid_argument("Profile name cannot be empty")); + } + + const MAX_IDENTIFIER_LENGTH: usize = 63; + let base_name = sanitize_table_name(&request.table_name); + if base_name.len() > MAX_IDENTIFIER_LENGTH { + return Err(Status::invalid_argument(format!( + "Identifier '{}' exceeds the {} character limit.", + base_name, + MAX_IDENTIFIER_LENGTH + ))); + } + let user_part_cleaned = request.table_name .replace(|c: char| !c.is_ascii_alphanumeric() && c != '_', "") .trim_matches('_') @@ -179,6 +193,9 @@ async fn execute_table_definition( if !is_valid_identifier(&col_def.name) { return Err(Status::invalid_argument("Invalid column name")); } + if col_name.ends_with("_id") || col_name == "id" || col_name == "deleted" || col_name == "created_at" { + return Err(Status::invalid_argument("Invalid column name")); + } let sql_type = map_field_type(&col_def.field_type)?; columns.push(format!("\"{}\" {}", col_name, sql_type)); } diff --git a/server/tests/table_definition/post_table_definition_test2.rs b/server/tests/table_definition/post_table_definition_test2.rs index 3e392c9..b08dc4f 100644 --- a/server/tests/table_definition/post_table_definition_test2.rs +++ b/server/tests/table_definition/post_table_definition_test2.rs @@ -434,7 +434,7 @@ async fn test_long_identifier_length(#[future] pool: PgPool) { ..Default::default() }; let err = post_table_definition(&pool, req).await.unwrap_err(); - assert_eq!(err.code(), Code::Internal); + assert_eq!(err.code(), Code::InvalidArgument); } // 15) Decimal precision overflow must be caught by our parser. diff --git a/server/tests/table_definition/post_table_definition_test3.rs b/server/tests/table_definition/post_table_definition_test3.rs index cc52bfd..a2ce498 100644 --- a/server/tests/table_definition/post_table_definition_test3.rs +++ b/server/tests/table_definition/post_table_definition_test3.rs @@ -173,26 +173,24 @@ async fn test_fail_on_true_self_referential_link(#[future] pool: PgPool) { #[tokio::test] async fn test_behavior_on_empty_profile_name(#[future] pool: PgPool) { // Scenario: Attempt to create a table with an empty profile name. - // Expected: This should violate the database's NOT NULL constraint on profiles.name. + // Expected: This should be rejected by input validation. let pool = pool.await; let request = PostTableDefinitionRequest { profile_name: "".into(), table_name: "table_in_empty_profile".into(), ..Default::default() }; - // Act let result = post_table_definition(&pool, request).await; - // Assert let err = result.unwrap_err(); assert_eq!( err.code(), - Code::Internal, - "Expected Internal error from DB constraint violation" + Code::InvalidArgument, // Changed from Internal + "Expected InvalidArgument error from input validation" ); assert!( - err.message().to_lowercase().contains("profile error"), + err.message().contains("Profile name cannot be empty"), // Updated message "Unexpected error message: {}", err.message() ); diff --git a/server/tests/table_definition/post_table_definition_test4.rs b/server/tests/table_definition/post_table_definition_test4.rs index 52890c0..39af472 100644 --- a/server/tests/table_definition/post_table_definition_test4.rs +++ b/server/tests/table_definition/post_table_definition_test4.rs @@ -4,41 +4,6 @@ // ========= Category 5: Implementation-Specific Edge Cases ========= -#[rstest] -#[tokio::test] -async fn test_column_name_with_id_suffix_is_allowed(#[future] pool: PgPool) { - // NOTE: This test confirms the CURRENT behavior of the code, which is that creating - // a column with an `_id` suffix is allowed. The existing test - // `test_fail_on_column_name_suffix_id` makes a contrary assumption. - // If this behavior is undesirable, the `is_valid_identifier` function or its - // usage in the handler should be updated to reject such names. - let pool = pool.await; - let request = PostTableDefinitionRequest { - profile_name: "default".into(), - table_name: "orders_with_custom_id".into(), - columns: vec![ColumnDefinition { - name: "legacy_order_id".into(), // A user-defined column ending in `_id` - field_type: "integer".into(), - }], - ..Default::default() - }; - - // Act - let response = post_table_definition(&pool, request).await.unwrap(); - - // Assert - assert!(response.success); - assert!(response.sql.contains("\"legacy_order_id\" INTEGER")); - - // Verify in the actual database - assert_table_structure_is_correct( - &pool, - "orders_with_custom_id", - &[("legacy_order_id", "integer")], - ) - .await; -} - #[rstest] #[tokio::test] async fn test_fail_on_fk_base_name_collision(#[future] pool: PgPool) { @@ -190,3 +155,68 @@ async fn test_fail_gracefully_if_schema_is_missing(#[future] pool: PgPool) { // Check for the Postgres error message for a missing schema. assert!(err.message().to_lowercase().contains("schema \"gen\" does not exist")); } + +#[rstest] +#[tokio::test] +async fn test_column_name_with_id_suffix_is_rejected(#[future] pool: PgPool) { + // Test that column names ending with '_id' are properly rejected during input validation + let pool = pool.await; + + // Test 1: Column ending with '_id' should be rejected + let request = PostTableDefinitionRequest { + profile_name: "default".into(), + table_name: "orders".into(), // Valid table name + columns: vec![ColumnDefinition { + name: "legacy_order_id".into(), // This should be rejected + field_type: "integer".into(), + }], + ..Default::default() + }; + + // Act & Assert - should fail validation + let result = post_table_definition(&pool, request).await; + assert!(result.is_err(), "Column names ending with '_id' should be rejected"); + if let Err(status) = result { + assert_eq!(status.code(), tonic::Code::InvalidArgument); + assert!(status.message().contains("Invalid column name")); + } + + // Test 2: Column named exactly 'id' should be rejected + let request2 = PostTableDefinitionRequest { + profile_name: "default".into(), + table_name: "orders".into(), + columns: vec![ColumnDefinition { + name: "id".into(), // This should be rejected + field_type: "integer".into(), + }], + ..Default::default() + }; + + let result2 = post_table_definition(&pool, request2).await; + assert!(result2.is_err(), "Column named 'id' should be rejected"); +} + +#[rstest] +#[tokio::test] +async fn test_table_name_with_id_suffix_is_rejected(#[future] pool: PgPool) { + // Test that table names ending with '_id' are properly rejected during input validation + let pool = pool.await; + + let request = PostTableDefinitionRequest { + profile_name: "default".into(), + table_name: "orders_id".into(), // This should be rejected + columns: vec![ColumnDefinition { + name: "customer_name".into(), // Valid column name + field_type: "text".into(), + }], + ..Default::default() + }; + + // Act & Assert - should fail validation + let result = post_table_definition(&pool, request).await; + assert!(result.is_err(), "Table names ending with '_id' should be rejected"); + if let Err(status) = result { + assert_eq!(status.code(), tonic::Code::InvalidArgument); + assert!(status.message().contains("Table name cannot be 'id', 'deleted', 'created_at' or end with '_id'")); + } +}