Skip to content
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

POC: Expand Patch Operations #465

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Qualizorg
Copy link

@Qualizorg Qualizorg commented Aug 16, 2024

Related to #442, #307, #297

Implementation to support replace, set, add, remove, increment patch operations, excluding move.

Both implementations follow expression + path, inspired by #307 .

Expression approach can't work with ArrayIndexExpression ( x => x.Address.Tags[0]), can make it work if it brings value, nevertheless same goal can be achieved over string path approach (/address/tags/0)

Questions

  1. InMemoryRepository needs rework to handle the other operation types, how critical is it?
  2. Decide if bulk patch operations per partition key are valuable.

Need feedback regarding approach and any major optimizations we can perform. @IEvangelist @mumby0168

Tasks

  • Rework Replace Operation + Unit Tests
  • Implemented Set Operation + Unit Tests
  • Implemented Add Operation + Unit Tests
  • Implemented Remove Operation + Unit Tests
  • Implemented Increment Operation + Unit Tests
  • Document methods with Microsoft Docs
  • Add all corresponding tests to DefaultRepositoryTests
  • Remove commented code (dependant on question 1)

@Qualizorg
Copy link
Author

Updated DefaultRepository.Update.cs to return etag as value.

@@ -1,55 +1,396 @@
// Copyright (c) David Pine. All rights reserved.
// Copyright (c) IEvangelist. All rights reserved.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright (c) IEvangelist. All rights reserved.
// Copyright (c) David Pine. All rights reserved.


internal class PatchOperationBuilder<TItem> : IPatchOperationBuilder<TItem> where TItem : IItem
[assembly: InternalsVisibleTo("Microsoft.Azure.CosmosRepositoryTests")]
namespace Microsoft.Azure.CosmosRepository.Builders
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File-scoped namespaces please.


public IReadOnlyList<PatchOperation> PatchOperations => _patchOperations;
internal class PatchOperationBuilder<TItem> : IPatchOperationBuilder<TItem> where TItem : IItem
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be sealed?

_patchOperations.Add(PatchOperation.Replace($"/{propertyToReplace}", value));
public PatchOperationBuilder() =>
_namingStrategy = new CamelCaseNamingStrategy();
public PatchOperationBuilder(CosmosPropertyNamingPolicy? cosmosPropertyNamingPolicy) =>
Copy link
Owner

@IEvangelist IEvangelist Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, prefer primary constructor.

Suggested change
public PatchOperationBuilder(CosmosPropertyNamingPolicy? cosmosPropertyNamingPolicy) =>
public PatchOperationBuilder(CosmosPropertyNamingPolicy? cosmosPropertyNamingPolicy) =>

return this;
}

private string NormalizePathPrefix(string path) => "/" + path.TrimStart('/').TrimEnd('/');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer string interpolation.

@Qualizorg
Copy link
Author

Qualizorg commented Aug 20, 2024

Add Operation doesn't work properly with lambda.

            var tenantId = Guid.Parse("fd048274-190c-4e50-90ff-f2bb84974523");
            var companyId = Guid.Parse("c4707548-5033-4c00-9735-9268c326c04a");

            // Create a company with the new properties
            var company = new Company
            {
                TenantId = tenantId,
                CompanyId = companyId,
                Id = companyId.ToString(),
                CompanyName = "Tech Innovations Inc.",
                Industry = "Software Development",
                Location = "Silicon Valley, CA",
                YearFounded = 2005,
                IsPublic = true,
                Revenue = 1200000000m, // 1.2 Billion USD
                Website = "https://www.techinnovations.com",
                Manager = new CompanyEmployee
                {
                    EmployeeId = 3,
                    FullName = "Johh Walker",
                    Position = "CEO",
                    Salary = 170000m,
                    EmployeeAddress = new Address
                    {
                        Street = "456 Oak St",
                        City = "aaaa",
                        State = "IL",
                        ZipCode = "62705"
                    }
                },
                Departments = new List<Department>
            {
                new Department
                {
                    DepartmentId = 101,
                    DepartmentName = "Engineering",
                    ManagerName = "Alice Johnson",
                    Employees = new List<CompanyEmployee>
                    {
                        new CompanyEmployee
                        {
                            EmployeeId = 1,
                            FullName = "John Doe",
                            Position = "Software Engineer",
                            Salary = 80000m,
                            EmployeeAddress = new Address
                            {
                                Street = "123 Main St",
                                City = "Springfield",
                                State = "IL",
                                ZipCode = "62704"
                            }
                        }
                    }
                },
                new Department
                {
                    DepartmentId = 102,
                    DepartmentName = "Marketing",
                    ManagerName = "Bob Williams",
                    Employees = new List<CompanyEmployee>
                    {
                        new CompanyEmployee
                        {
                            EmployeeId = 2,
                            FullName = "Jane Smith",
                            Position = "Marketing Specialist",
                            Salary = 70000m,
                            EmployeeAddress = new Address
                            {
                                Street = "456 Oak St",
                                City = "Springfield",
                                State = "IL",
                                ZipCode = "62705"
                            }
                        }
                    }
                }
            }
            };
  await _testRepository.PatchAsync(companyId, tenantId, builder =>
      builder
      .Remove(x => x.Industry)
      .Replace(x => x.CompanyName, "CompanyNameTest")
      .Set(x => x.Manager, new CompanyEmployee() { EmployeeId = 4, FullName = "Paul Walker", Position = "CTOO", Salary = 4560000m })
      .Add(x => x.Departments, [new Department() { DepartmentId = 1234, DepartmentName = "Tech Dep", ManagerName = "Terry Springfield" }])
  );

Cosmos exception
Cannot deserialize the current JSON array (e.g. [1,2,3]) into type 'QV.OrganisationsComponent.Domain.Department' because the type requires a JSON object (e.g. {"name":"value"}) to deserialize correctly.\r\nTo fix this error either change the JSON to a JSON object (e.g. {"name":"value"}) or change the deserialized type to an array or a type that implements a collection interface (e.g. ICollection, IList) like List<T> that can be deserialized from a JSON array. JsonArrayAttribute can also be added to the type to force it to deserialize from a JSON array.\r\nPath 'departments[2]', line 1, position 703.

This is due the fact that x => x.Departments is a list and TValue from .Add expects it to be a list.

Needs more work.

Edit: Format Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants