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

Expanding Patch Operations #307

Closed
wants to merge 2 commits into from

Conversation

evancjohnson
Copy link
Contributor

This PR expands the patch operations to support Set, Add, Remove, and Increment.

@mumby0168 I understand this was assigned to you, I hope I am not stepping on your toes, this is just a draft to make sure I haven't overstepped, I will add tests after I know if this PR is needed.

I also had a question about the _rawPatchOperations is it needed?

Fixes #297

@IEvangelist
Copy link
Owner

This looks awesome, thank you for this contribution @evancjohnson! As long as @mumby0168 is good with this, let's make it happen. Yes, please add tests. As for the _rawPatchOperations I'm not certain, Billy would know.

@IEvangelist
Copy link
Owner

@all-contributors please add @evancjohnson for code and tests. Thank you 💜

@allcontributors
Copy link
Contributor

@IEvangelist

I've put up a pull request to add @evancjohnson! 🎉

@evancjohnson evancjohnson marked this pull request as ready for review November 7, 2022 08:09
@evancjohnson
Copy link
Contributor Author

@mumby0168 @IEvangelist I see where _rawPatchOperations is used, the current implementation of InMemoryRepository expects property info to be provided in the internal patch operation. Is this approach reasonable given that the caller can also provide an explicit path? Using the direct path would allow users to patch a nested object.

Should the InMemoryRepository use Newtonsoft.Json in some way (I haven't actually looked up if this possible, I believe I have done something like this in the past) to apply the replace operation to the TItem by path removing the need for _rawPatchOperations and instead leaning on the information provided by the PatchOperation

@mumby0168
Copy link
Collaborator

Hi @evancjohnson yes, from my distant memory the _rawPatchOperations allowed the in-memory repo to support this feature, it was a challenge and I suspect keeping the support for all the patch operations maybe be difficult ... if there is a way to apply the modifications to JSON from the in memory repo that would be great! Check out the json path support on the JObject type that might help in this scenario?

@evancjohnson evancjohnson changed the title POC: Expanding Patch Operations Expanding Patch Operations Nov 8, 2022
@evancjohnson
Copy link
Contributor Author

Thanks for the tip @mumby0168. I will do some research on a good approach. I've got a quick questions.

How rigid is the InMemoryRepository? Is it expected to perform exactly the same way as CosmosDB?

@IEvangelist
Copy link
Owner

Thanks for the tip @mumby0168. I will do some research on a good approach. I've got a quick questions.

How rigid is the InMemoryRepository? Is it expected to perform exactly the same way as CosmosDB?

Hi @evancjohnson - it's not super rigid. It was intended to be a helper so that you could use it for unit testing, to help ensure other functionality that might rely on the underlying data store. Does that make sense? We know that there are functional gaps, and we're ok with that for now.

@evancjohnson
Copy link
Contributor Author

@IEvangelist I'll have time to work on this during the weekend, sorry for leaving it open for so long.

@mateuszkumpf
Copy link

@IEvangelist I'll have time to work on this during the weekend, sorry for leaving it open for so long.

Will you be able to finish working on this code?

@IEvangelist
Copy link
Owner

@IEvangelist I'll have time to work on this during the weekend, sorry for leaving it open for so long.

Will you be able to finish working on this code?

Hi @MatKumpf - this was something that @evancjohnson was working on, but hasn't updated. If the op is unable to finish this, I could work on it. Unless it's something that you're interested in?

@mateuszkumpf
Copy link

mateuszkumpf commented Mar 15, 2023

@IEvangelist I'll have time to work on this during the weekend, sorry for leaving it open for so long.

Will you be able to finish working on this code?

Hi @MatKumpf - this was something that @evancjohnson was working on, but hasn't updated. If the op is unable to finish this, I could work on it. Unless it's something that you're interested in?

I won't conceal, it would be one of the most important functionalities for me 😄 So far I have to in few places create my custom connection with container and use the rest of patch operations on it.

Edit: I forgot to answer your question. This is the functionality I would like but I'm not sure I fully understand the dependencies that are in this project so I wouldn't take it on myself for the time being :D

@mateuszkumpf
Copy link

Furthermore, as @mumby0168 rightly points out, supporting all operations in an in-memory repository can be quite difficult and the question is what concessions can be made.
Whether we can, for example, remove or replace a field depends on whether it is physically in an object in the database. Whether it is there is influenced, for example, by the choice of serialization method or initialisation with a default value (not necessarily default(T)).
These are implementation decisions I prefer not to make :)

@evancjohnson
Copy link
Contributor Author

@MatKumpf @IEvangelist Sorry for the delay friends I have just finished a project and am about to graduate school, so I have much more time. I am pretty close to finishing this one up. I do have a question which is more architectural for David and Billy. Should test host support patch operations? I personally do not believe that it should, and those tests should be run against the emulator.

The reason I suggest that due to the fact patch operations have some not at first noticeable quirks. Check the troubleshooting for an example.

What are the thoughts on just supporting testing of patch operations internally and not exposing those in the test harness provided by the library?

@mumby0168
Copy link
Collaborator

@MatKumpf @IEvangelist Sorry for the delay friends I have just finished a project and am about to graduate school, so I have much more time. I am pretty close to finishing this one up. I do have a question which is more architectural for David and Billy. Should test host support patch operations? I personally do not believe that it should, and those tests should be run against the emulator.

The reason I suggest that due to the fact patch operations have some not at first noticeable quirks. Check the troubleshooting for an example.

What are the thoughts on just supporting testing of patch operations internally and not exposing those in the test harness provided by the library?

This is a decision we've made with some of the other features, as in not to support those features in the in memory implementation I think that is also fine in this case due to the complexity, would you agree @IEvangelist ?

@IEvangelist
Copy link
Owner

@MatKumpf @IEvangelist Sorry for the delay friends I have just finished a project and am about to graduate school, so I have much more time. I am pretty close to finishing this one up. I do have a question which is more architectural for David and Billy. Should test host support patch operations? I personally do not believe that it should, and those tests should be run against the emulator.
The reason I suggest that due to the fact patch operations have some not at first noticeable quirks. Check the troubleshooting for an example.
What are the thoughts on just supporting testing of patch operations internally and not exposing those in the test harness provided by the library?

This is a decision we've made with some of the other features, as in not to support those features in the in memory implementation I think that is also fine in this case due to the complexity, would you agree @IEvangelist ?

Hi @mumby0168 and @evancjohnson - I agree with Billy. The in-mem bits are just to help with unit testing, and not intended to be feature complete.

@evancjohnson
Copy link
Contributor Author

@IEvangelist @mumby0168 Thank you for getting back to me. I will update the tests and have the in memory repo throw a NotSupportedException when attempting to run the patch operations!

@IEvangelist
Copy link
Owner

Hi again @evancjohnson - any updates on this?

@IEvangelist
Copy link
Owner

We can always reopen this PR. But it's been nearly a year since it was first introduced. And months since the OP has replied.

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.

Exapand IPatchOperationBuilder to support Increment
4 participants