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

Bug on DataService for Azure Mobile Services.js #18

Open
akaztp opened this issue Feb 6, 2015 · 8 comments
Open

Bug on DataService for Azure Mobile Services.js #18

akaztp opened this issue Feb 6, 2015 · 8 comments

Comments

@akaztp
Copy link

akaztp commented Feb 6, 2015

On the function "_createErrorFromResponse" the before-last line is:

proto._catchNoConnectionError(err);

which gave me undefined error for "proto" in case of bad communications.
In fact, I don't find any variable "proto" in scope of this function or its parent.
I've changed this to:
ctor.prototype._catchNoConnectionError(err);

And now it works fine.
This bug is also on the ShaprePoint DataService, although I did not test, since I do not use that. But it seems like the same problem and solution applies.

Regards
ztp

@akaztp akaztp changed the title Bug on DataService for Aazure Mobile Services.js Bug on DataService for Azure Mobile Services.js Feb 6, 2015
wardbell added a commit that referenced this issue Feb 11, 2015
@wardbell
Copy link
Member

I think I fixed it (albeit in a different way than you suggested). Would you mind confirming for your app by pulling the latest source file and creating the connection error?

When you confirm it, I'll update the bower and npm packages

@akaztp
Copy link
Author

akaztp commented Feb 12, 2015

Yeap, it's good. thank you.
Meanwhile I've got another problem, and maybe it is related to these files. Before I open another issue, there is one thing I need to know. If it confirms my suspictions, you may want to hold on those updates to the packages.
When saving changes for an entity (EntityManager.saveChanges()), a PATCH request is sent to the Azure Mobile Services. This request only includes the key and the changed properties of the entity.
The question is: in this case, the "OK" response from the server should have values for all properties or can Breeze handle a response with only the key and the changed properties' names and values?
thx
ztp

@wardbell
Copy link
Member

I believe that the adapter was written with that in mind. It's been a long time.

Have you tried the azure mobile services sample (todo-zumo) in the breeze.js.samples repo? It does full CRUD and I'm sure I would have noticed if I had neglected that. Would help me if you can confirm or deny.

Thanks

@akaztp
Copy link
Author

akaztp commented Feb 12, 2015

I've just run the todo-zumo demo app. It does not show "my" problem because it gets all data from the server right after is saves the changes. Network inspection also shows that the response from the server for the PATCH command only returns the uploaded properties for the entity.
I've modified the todo-zumo, thus disabling the "refresh all" after the save and "my" problem now appears exactly. For the local entity, it resets to the defaults all properties that were not saved due to no changes: since it cannot find them on the server response.
I don't think it has anything to do with the Azure and abstract adapters.

Is it mandatory (by design of breeze) that I make a GET after a PATCH for some entity? In high concurrent systems, I can see the reason, but I can see many cases where it shouldn't be mandatory.

Should I open an issue on the main breeze repo?

@wardbell
Copy link
Member

No. This is a breeze labs concern given that breeze core has no zumo support at all. This is the right place to deal with it.

I don't think of this as a bug. It's a feature question.

Let's think about what would be desirable. Then we (you and I) can create an adapter to meet that need. I know how and even know what I would do. But I'm more interested in what you think.

So lets take the time to hash out a proposal here. Maybe you'd like to start a design doc on Google docs? We can reference it here. Your call.

@akaztp
Copy link
Author

akaztp commented Feb 13, 2015

Ward, thank you one more time for spending some time on this.

Either me or you are not understanding each other. Or both! :)

I don’t see this as a feature question/request.
Since I've progressed a lot in knowing what is going on, I will re-state the problem I’ve found and we will go from there.

The problem happens when the Azure Mobile Services dataservice sends a PATCH request with only the modified changes to an entity AND, afterwards, the client application, by design, does not go GET the entity (with all its properties).
The problem arises because the Breeze core does not expect a partial result for an entity. More precisely the code on EntityType._updateTargetFromRaw() and DataProperty.getRawValueFromServer() expect all properties to be on the Raw object or else they will set them to their default values. And their default values will override the cached ones.

I don’t know what the right solution is, mainly because I don’t know the side effects of changing updateTargetFromRaw() so it would check for the presence of each property on the raw data.

One solution is to send all properties in the PATCH (except for the key). I had success, modifying the Azure Mobile Services dataservice, namely on the _createChangeRequest(), line 161:

  • rawEntity = helper.unwrapChangedValues(entity, entityManager.metadataStore, adapter._transformSaveValue);
  • rawEntity = helper.unwrapInstance(entity, adapter._transformSaveValue);
  • delete rawEntity[type.keyProperties[0].name];

Another one, I think, would be to add server side code to the Azure Mobile Services to not return the changed values on a PATCH request, or return all of them. But I don’t even know if this is possible to code there.

A little aside from this problem, I would like to see support to the concurrency problem (in your sample solved by the GET after a PATCH) by examining a serial/timestamp property and comparing it to the cached version. But surely there would be a lot more to say about this.

Regards
ztp

@wardbell
Copy link
Member

I'm looking for what you would like to do, not the solution. Ideally you wouldn't change a thing on the server nor do any special gymnastics on the client.

I realize that updateRaw... etc are unsuitable for blending a few values from the server with unchanged values on the client. That's where I ride in on my white horse.so don't worry about how. Just think about what.

Your comment on optimistic concurrency is along those lines. Do you have a property for that? Shouldn't we use the etag? Or does that not actual work in zumo? That's the stuff I need help with.

Cheers, W

@akaztp
Copy link
Author

akaztp commented Feb 16, 2015

My current project is not that much OCC prone, so I just wanted the zumo dataservice to work as (I think it is) expected.
But one of the goals of my project is to make me get experience in SPAs and make a kind-of proof of capabilities for my freelance developer venture. In fact, there is one aspect of my application that would be nice to support updating of some entities from different clients at the same time. I think now I understand where you want to get at and I’m pleased to be able to make part of it.

The Azure Mobile Services implements OCC with the ETag, as this article explains:
http://blogs.msdn.com/b/carlosfigueira/archive/2013/12/02/accessing-optimistic-concurrency-features-in-azure-mobile-services-client-sdks.aspx
In summary, provide in the querystring “__systemproperties=__version” and the ETag mechanism will be activated.

What would be neat is breeze to interface to that accordingly:

  • inform the client app of failed updates because of concurrency violation
    • also, provide the up-to-date entity to the client (if returned from the server). Otherwise the client must make some gymnastics to retrieve the current entity without overriding the cached one, if wanting to compare the two and/or somehow present them to the user.
  • automatically manage the OOC features through HTTP ETag

I’m aware that Breeze:

  • has some support for ETags (passes it between the entity’s extraMetadata and the OData default adapter)
  • supports “concurrencyProperties” (although I don’t quite understand why it tries to increment/update them when saving to the server)
  • discards entities returned by server errors

It would be desirable to:

  • correlate the ETags to concurrencyProperties.
  • on entity update to the server:
    • no conflict: breeze must incorporate the partial results into the cached entity
    • conflict: breeze update should fail with the most recent values provided by the server:
      • It is up to the client to respond accordingly: update itself or re-issue the update to the server.
      • breeze should have an easy method to update the cached version with the recent results. Here we will have problems because of navigation properties: they can be updated by “side-effect” and the client application didn’t ask for it in the first place and so they may be out of its context.

Is this what you were thinking?
Regards
ztp

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

No branches or pull requests

2 participants