unify api for simple use #2
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThe recent updates streamline the casting and validation processes in the schema management system, enhancing efficiency and error handling. Key modifications include renaming functions for clarity, integrating similar logic into single functions, and refining error structures for better clarity. These changes aim to simplify the codebase, improve performance, and make error messages more informative and easier to understand. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lib/schema.ex (1 hunks)
- lib/schema_helper.ex (1 hunks)
- lib/skema.ex (8 hunks)
- test/defschema_test.exs (3 hunks)
- test/skema_test.exs (15 hunks)
- test/validate_test.exs (8 hunks)
Additional comments: 22
test/validate_test.exs (10)
- 1-8: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [5-17]
The test "validate nested map with success" correctly demonstrates the validation of a nested map structure. The schema definition and the assertion are clear and follow best practices.
- 14-23: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [20-33]
The test "validate nested map with bad value should error" effectively checks for validation errors when the nested map structure does not match the schema. The use of a nested
errorskey in the assertion is consistent with the PR's objectives to refine error handling.
- 30-39: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-49]
The test "validate nested map with bad nested value should error" accurately tests for errors in nested values. The assertion structure, with a nested
errorskey, aligns with the enhanced error handling approach.
- 46-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-66]
The test "validate nested map skip nested check if value nil" demonstrates the library's behavior when optional nested structures are not provided. This test ensures that the validation logic correctly handles
nilvalues for optional nested maps.
- 63-72: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [69-83]
The test "validate nested map skip nested check if key is missing" checks the library's handling of missing optional nested structures. It ensures that the absence of optional keys does not result in validation errors, which is a crucial aspect of flexible schema validation.
- 80-89: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-101]
The test "validate array of nested map with valid value should ok" correctly validates an array of nested maps against the schema. This test ensures that the library can handle complex nested structures within arrays.
- 98-107: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [104-119]
The test "validate array of nested map with invalid value should error" effectively checks for validation errors within arrays of nested maps. The structured error response with a nested
errorskey is consistent with the PR's objectives for improved error handling.
- 122-122: The custom validation function
validate_emailis correctly implemented to validate email addresses using a regular expression. This function demonstrates the library's support for custom validation logic.- 130-134: The test "validate with custom function ok with good value" correctly demonstrates the use of a custom validation function. It ensures that custom validation logic can be integrated seamlessly into the validation process.
- 138-142: The test "validate with custom function error with bad value" effectively tests the error handling of a custom validation function. The structured error response aligns with the PR's objectives for refined error handling.
test/skema_test.exs (12)
- 155-155: The schema definition for
numberhas been updated to includetype: :integerinstead of[:integer, number: [min: 5]]. This change simplifies the schema but removes the minimum value constraint. Ensure that this change aligns with the intended validation logic.- 253-253: The test case for a custom function returning a custom error message (
cast_and_validate func return custom message) is correctly implemented. It demonstrates how a custom error message can be returned from a casting function, aligning with the PR's objective to refine error handling.- 306-306: The error structure for embedded validation errors has been updated to include an additional level of nesting with a unified
errorskey. This change improves error organization and consistency across the test suite. Ensure that all related code and tests have been updated to reflect this new error structure.- 317-317: The test for missing required values correctly uses the updated error structure, demonstrating the library's ability to handle and report errors in a consistent and organized manner. This aligns with the PR's objective to improve error handling.
- 388-388: The test for custom validation messages demonstrates the library's flexibility in providing meaningful error messages. This aligns with the PR's objective to enhance error handling and user feedback.
- 402-402: The test case for returning cast_and_validate errors prioritizes casting errors over validation errors, which is a logical approach to error handling. This ensures that data is correctly formatted before applying validation rules.
- 411-411: The test for returning both cast_and_validate errors and validation errors for fields demonstrates the library's comprehensive error handling capabilities. This is crucial for providing detailed feedback to the user.
- 426-433: The test for nested schema validation errors showcases the library's ability to handle complex data structures and provide detailed error messages. This aligns with the PR's goal of improving error handling and making the API more intuitive.
- 442-442: The test for invalid data types in nested schemas correctly identifies and reports errors, demonstrating the library's robust validation capabilities. This ensures that data structures conform to the expected schema.
- 449-449: The test for array type mismatches correctly identifies and reports errors when a map is provided instead of an array. This aligns with the PR's objective to refine error handling and ensure data integrity.
- 460-460: The test for validating array items with errors demonstrates the library's ability to handle complex validation scenarios and provide meaningful feedback. This is crucial for ensuring data quality and integrity.
- 472-484: The tests for dynamic require validation showcase the library's flexibility in applying validation rules based on dynamic conditions. This enhances the library's usability and adaptability to different use cases.
| @@ -247,7 +238,7 @@ defmodule ParamTest do | |||
| end | |||
|
|
|||
| test "cast_and_validate func with 3 arguments return error" do | |||
| assert {:error, %{name: ["bad function"]}} = | |||
| assert {:error, %{errors: %{name: ["bad function"]}}} = | |||
There was a problem hiding this comment.
The test case for a custom function with an incorrect number of arguments (cast_and_validate func with 3 arguments return error) correctly expects an error. However, the function provided in the test case does not match the expected error scenario description. The function returns {:ok, value} regardless of the input, which does not align with the test's intent to demonstrate a function error due to an incorrect number of arguments.
- cast_func: fn value, _data, _name ->
+ cast_func: fn value, _data, _name -> # This function should demonstrate an error scenario due to incorrect arguments.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.
| assert {:error, %{errors: %{name: ["bad function"]}}} = | |
| assert {:error, %{errors: %{name: ["bad function"]}}} = |
| ] | ||
| } | ||
|
|
||
| assert {:error, %{user: [%{name: ["is required"]}]}} = Skema.cast_and_validate(data, @array_schema) | ||
| assert {:error, %{errors: %{user: [%{errors: %{name: ["is required"]}}]}}} = | ||
| Skema.cast_and_validate(data, @array_schema) | ||
| end | ||
|
|
||
| test "error with custom message" do | ||
| schema = %{ | ||
| age: [type: :integer, number: [min: 10], message: "so khong hop le"] | ||
| } | ||
|
|
||
| assert {:error, %{age: ["so khong hop le"]}} = Skema.cast_and_validate(%{"age" => "abc"}, schema) | ||
| assert {:error, %{errors: %{age: ["so khong hop le"]}}} = Skema.cast_and_validate(%{"age" => "abc"}, schema) | ||
| end | ||
|
|
||
| test "cast_and_validate validate required skip if default is set" do |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [363-380]
The addition of a new test for array validation with the @tag :only directive is a positive change, as it focuses on a specific area of functionality. However, ensure that the @tag :only directive is removed or commented out before merging to avoid limiting the test suite's scope during automated testing.
- @tag :only
+ # @tag :only # Ensure to remove or comment out before merging.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lib/schema.ex (1 hunks)
- lib/skema.ex (7 hunks)
- test/defschema_test.exs (7 hunks)
- test/skema_test.exs (15 hunks)
Files skipped from review as they are similar to previous changes (2)
- lib/schema.ex
- test/defschema_test.exs
Additional comments: 22
lib/skema.ex (8)
- 54-57: The handling of schemas defined as atoms in
cast/2is a good practice for supporting modular schemas. This approach allows for cleaner code organization by defining schemas in separate modules. However, ensure that the__fields__function is implemented in all schema modules to avoid runtime errors.- 71-82: The implementation of
cast/1usingEnum.reduceto iterate over schema fields and apply casting logic is efficient and clean. However, it's important to ensure thatResult.put_dataandResult.put_errorhandle concurrency correctly ifcast/1could be called from multiple processes simultaneously, to avoid data races.Consider verifying the thread-safety of
Result.put_dataandResult.put_errorif applicable to your use case.
- 86-88: The delegation of validation to
__fields__when the schema is an atom simplifies the handling of modular schemas, similar to the casting function. This consistency in handling atom-based schemas across casting and validation functions is commendable.- 99-115: The
validate/1function's logic, which skips fields with existing errors, is a good practice for efficiency and preventing redundant validations. However, ensure that the error handling logic inResult.put_erroris robust and clearly communicates the nature of validation errors to the end-users.- 128-145: The
transform/1function follows a similar pattern tocast/1andvalidate/1, which is good for consistency. However, the same considerations regarding concurrency and error handling apply here as well. Ensure that transformations that could potentially alter shared state are handled safely.Review the thread-safety and error handling of transformations, especially if they involve shared state or external resources.
- 220-220: Simplifying nested map casting in
cast_value/2by directly callingcast/2is a clean and efficient approach. This change reduces complexity and potential for errors by reusing existing logic for casting.- 279-279: The handling of nested map validation in
do_validate/4by callingvalidate/2directly is a good practice, as it reuses existing validation logic for nested structures. This approach ensures consistency in validation across different levels of nested data.- 322-322: The
collect_validation_result/1function's approach to aggregating validation errors is efficient and ensures that all errors are captured and returned in a consistent format. This is crucial for providing clear feedback to users about validation issues.test/skema_test.exs (14)
- 155-155: The test
"schema short hand"has been updated to include a more complex schema definition. This change ensures that the test suite covers scenarios with nested schema definitions and validations. It's important to ensure that all edge cases are covered for nested schemas.- 231-231: The test case for a custom function with an incorrect number of arguments correctly expects an error. However, the function provided in the test case does not match the expected error scenario description. The function returns
{:ok, value}regardless of the input, which does not align with the test's intent to demonstrate a function error due to an incorrect number of arguments.- 243-243: The test case for a custom function returning a custom error message is well-designed. It verifies that custom error messages are correctly handled and returned by the validation logic. This is important for providing clear and specific feedback to users.
- 296-296: The test case for embedded validation with invalid values correctly checks for nested error structures. This approach is consistent with the updated error handling logic in the main codebase, ensuring that errors are properly nested and reported.
- 307-307: The test for missing required values in an embedded schema is crucial for ensuring that required validations are correctly applied at all levels of nested data. This test verifies that the error messages are appropriately nested and clear.
- 366-381: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [353-370]
The addition of a new test for array validation with the
@tag :onlydirective is a positive change, as it focuses on a specific area of functionality. However, ensure that the@tag :onlydirective is removed or commented out before merging to avoid limiting the test suite's scope during automated testing.
- 378-378: The test for custom error messages in validation ensures that the library supports and correctly handles custom validation messages. This is important for flexibility and providing user-friendly error messages.
- 392-392: The test for prioritizing cast errors over validation errors is important for ensuring that data integrity issues are addressed before validation logic is applied. This approach helps in identifying and fixing fundamental issues with data formatting or types early in the process.
- 401-401: The test for returning both cast and validation errors for fields with valid casting but invalid validation logic is crucial. It ensures that users receive comprehensive feedback on all issues with their data, not just the first encountered error.
- 416-423: The test for nested schema validation errors provides a thorough check for the handling of complex data structures. It ensures that errors are correctly nested and reported, which is essential for debugging and fixing data issues in complex applications.
- 432-432: The test for invalid data types in nested schemas ensures that the library correctly identifies and reports type mismatches. This is important for maintaining data integrity and preventing unexpected behavior in applications using the library.
- 439-439: The test for array type validation when given a map instead of an array is important for ensuring type safety. It verifies that the library correctly identifies and rejects incorrect data types, maintaining the integrity of the data structure.
- 450-450: The test for validating array items with errors ensures that item-level validations within arrays are correctly applied and reported. This is crucial for data structures that contain collections of items, each needing to meet specific criteria.
- 462-474: The tests for dynamic required validation demonstrate the library's flexibility in applying validation logic based on dynamic conditions. This feature is important for complex validation scenarios where the requirements may depend on other parts of the data.
lib/skema.ex
Outdated
| with {_, {:ok, data}} <- {:cast, cast(data, schema)}, | ||
| {_, :ok} <- {:validate, validate(data, schema)} do | ||
| {:ok, data} | ||
| else | ||
| {:cast, {:error, result}} -> | ||
| # valiate valid data to get more detail error | ||
| validate(result) | ||
|
|
||
| [schema: schema, params: data] | ||
| |> Result.new() | ||
| |> cast() | ||
| |> validate() | ||
| |> transform() | ||
| |> case do | ||
| %Result{valid?: true, valid_data: valid_data} -> {:ok, valid_data} | ||
| %{errors: errors} -> {:error, errors} | ||
| {:validate, error} -> | ||
| error |
There was a problem hiding this comment.
The refactoring of cast_and_validate/2 to handle casting and validation separately using with improves error handling by clearly separating concerns. However, when handling the {:cast, {:error, result}} case, it directly calls validate(result) without specifying what should be done with the validation result. This could lead to unhandled errors or unexpected behavior if validation fails.
Consider handling the validation result explicitly, either by aggregating errors or by returning them in a consistent format with the rest of the function.
# Suggested change to handle validation result explicitly
{:cast, {:error, result}} ->
case validate(result) do
:ok -> {:error, result} # or another appropriate action
{:error, validation_errors} -> {:error, validation_errors} # Aggregate or handle errors
end
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- README.md (1 hunks)
- lib/schema.ex (4 hunks)
- lib/schema_helper.ex (2 hunks)
- lib/skema.ex (8 hunks)
- mix.exs (1 hunks)
Files skipped from review due to trivial changes (1)
- mix.exs
Files skipped from review as they are similar to previous changes (2)
- lib/schema.ex
- lib/schema_helper.ex
Additional comments: 1
README.md (1)
- 58-58: The change from
IndexParams.cast(params)toSkema.cast(params, IndexParams)in theindexfunction example aligns with the PR's objective to simplify the API. Ensure that the rest of the documentation and examples are updated to reflect this new usage pattern for consistency and to avoid confusion for new users.
Summary by CodeRabbit
Skemainto a schema validation and casting library for Elixir.