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

Delete after Patch #182

Open
phatcher opened this issue Dec 9, 2015 · 9 comments
Open

Delete after Patch #182

phatcher opened this issue Dec 9, 2015 · 9 comments
Assignees

Comments

@phatcher
Copy link
Collaborator

phatcher commented Dec 9, 2015

I've read the documentation where it says that you can't delete links in the same call as the patch, but I think there a problem with the logic.

Say I have a model as follows..
class OrderLine
{
int Id { get; set; }
int ProductId { get; set; }
Product Product { get; set; }
int Quantity { get; set; }
decimal Price { get; set; }
}

class Product
{
int Id { get; set; }
string Name { get; set; }
}

If I do a get on OrderLine without expanding Product, edit the values and then call Set passing the changed entity and call UpdateEntryAsync then I get two calls

  1. Patch which pushes the values and returns OK
  2. Delete OrderLine/$links/Product which returns a NotFound

This happens even if the model is marked up with a ForeignKey attribute.

I think this is happening as it's just using all the properties and sees that the Product is null, but to work around this means I have to develop my own version of the patch logic for client-side use.

Any suggestions?

@object object self-assigned this Dec 10, 2015
@object
Copy link
Member

object commented Dec 10, 2015

Thank you for the observation. Let me investigate it further and come back to you.

@phatcher
Copy link
Collaborator Author

phatcher commented Feb 7, 2016

Any progress?

@object
Copy link
Member

object commented Feb 16, 2016

Sorry didn't have time to look into it because of work pressure. Hope to dig into it during this week.

@object object added the bug label Feb 23, 2016
@object object added this to the 4.17. milestone Feb 23, 2016
@object object added question and removed bug labels Feb 23, 2016
@object object removed this from the 4.17. milestone Feb 23, 2016
@object
Copy link
Member

object commented Feb 23, 2016

Well, I first though this might be a bug, but after some thinking I believe this behavior is proper but you can still achieve what you need.

Since Simple.OData.Client operates with POCO types, it has no knowledge about client's intention apart from what can be obtained from the entity property values. It runs updates in smart mode, trying to take care of relations. When there is an OrderLine instance with null Product, it believes that the link should be removed, it doesn't know if this instance was fetched without expanding Product, how can it know from a POCO data? So it takes care of clearing this link.

How this behavior can be changed? There are two options.

1 (bad one). Define another class OrderLineWithoutProductLink that will not contain Product property (it should still contain ProductId) and run updates using this type.
2 (recommended one). Don't send the whole OrderLine instance on Set, send just the fields that are updated. This should trigger PATCH request for only actually updated properties.

Hope this helps.

@phatcher
Copy link
Collaborator Author

It makes the strongly-typed interface not so useful since I have to take the entity I have and turn it into a dictionary and then remove the bits I don't want - which I can do, but it's a lot of infrastructure code.

What might be nice is an overload of Patch on the strongly-typed interface that takes an Ignore list of properties that should not be patched; I'll take a look at the code and possibly propose a patch.

I do have some metadata available since I have ForeignKey attributes on the appropriate parts, so once the first idea is available, a little strategy pattern can provide a list of these sort of properties

@object
Copy link
Member

object commented Feb 23, 2016

I don't think it's a lot of infrastructure code because it may look like this:

client.For(...).Key(productID).Set(new { ProductName="abc", UnitPrice = 456m }).UpdateEntryAsync();

But you will indeed lose here strong typing by using anonymous types. An alternative is extend API with ignore (or opposite - include) option that would instruct the library to take into account only certain properties.

@phatcher
Copy link
Collaborator Author

My specific use-case is that I'm writing an MVC app over an OData API with 30-40 entities (and growing) so the MVC controller is dealing with entities, I'm using the same POCOs both sides, and then I have a generic MVC controller which is using client.For....

I think extending the API is the way to go, as then I can store the ignore/exclude list in the controller and pass it to the client call.

@object
Copy link
Member

object commented Feb 23, 2016

My problem with this is that I want to keep the API simple, and extending it with a new clause for relatively narrow scenario is something I would avoid if there are other options. So let's think more about it.

@object
Copy link
Member

object commented Feb 23, 2016

I think if API should be extended with support for this, it should be done simultaneosly to resolve this issue: #127 (deep insert). Although this is about different stuff, but also about extendind Set semantics.

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

No branches or pull requests

2 participants