-
Notifications
You must be signed in to change notification settings - Fork 3
Enhance typed client methods and validation #190
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?
Conversation
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.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
ecce02b to
65d7322
Compare
80e4556 to
24a2816
Compare
| private bool AreTypesCompatible(string csDataType, string schemaDataType) | ||
| { | ||
| // Exact match | ||
| if (string.Equals(csDataType, schemaDataType, StringComparison.OrdinalIgnoreCase)) |
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.
Does this correctly handle compatibility between float/double in C# and numbers in Weaviate? Or for example C# enums to Weaviate text? Can also be added in future releases
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.
If you give me more concrete use cases I can create tests for these scenarios
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.
Tried these additional tests in TestTypeValidator.cs:
// Models for compatibility tests
private class ProductFloat { public float Price { get; set; } }
private class ProductDecimal { public decimal Price { get; set; } }
private class OrderLong { public long Quantity { get; set; } }
private enum TaskStatusEnum { Pending, InProgress, Completed }
private class UserTask { public TaskStatusEnum Status { get; set; } }
[Theory]
[InlineData(typeof(ProductFloat))]
[InlineData(typeof(ProductDecimal))]
public void ValidateType_NumberCompatibility_IsValid(Type type)
{
// Arrange
var schema = new CollectionConfig
{
Name = "Product",
Properties =
[
new Property { Name = "price", DataType = [DataType.Number] }
]
};
var validator = TypeValidator.Default;
// Act
var result = validator.ValidateType(type, schema);
// Assert
Assert.True(result.IsValid);
Assert.Empty(result.Errors);
}
[Fact]
public void ValidateType_IntCompatibility_IsValid()
{
// Arrange
var schema = new CollectionConfig
{
Name = "Order",
Properties =
[
new Property { Name = "quantity", DataType = [DataType.Int] }
]
};
var validator = TypeValidator.Default;
// Act
var result = validator.ValidateType<OrderLong>(schema);
// Assert
Assert.True(result.IsValid);
Assert.Empty(result.Errors);
}
[Fact]
public void ValidateType_EnumCompatibility_IsValid()
{
// Arrange
var schema = new CollectionConfig
{
Name = "UserTask",
Properties = new List<Property>
{
new Property { Name = "status", DataType = new List<string> { DataType.Text } }
}
};
var validator = TypeValidator.Default;
// Act
var result = validator.ValidateType<UserTask>(schema);
// Assert
Assert.True(result.IsValid);
Assert.Empty(result.Errors);
}The first two worked 👏🏻 but the third one doesn't ValidateType_EnumCompatibility_IsValid. Maybe it makes sense to add enums mapping to text in DataTypeForType
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.
Yes, that's a very good idea.
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 updated the PR with the logic to handle Enums in both numerical and string forms. Check it out!
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.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
… performance and readability
…tion tests for validation
- Implemented unit tests for CollectionClientExtensions, TypedDataClient, TypedGenerateClient, TypedQueryClient, ensuring proper functionality and type safety. - Added extension methods for CollectionClient to support typed operations, including validation against collection schemas. - Introduced validation extensions to validate C# types against collection schemas, throwing exceptions on mismatches. - Enhanced test coverage for various scenarios, including insertion, fetching, and type validation for articles and products.
…validation scenarios
…ler for consistency
241026e to
34bcd89
Compare
Improve type safety and validation for typed clients, add integration and unit tests, and optimize the test workflow. Introduce optional client-side schema validation and enhance the TypeValidator to support anonymous types. Refactor code for better performance and readability.