The AITC client is implementing "tombstones" for deleted apps, by setting the "deleted" field on the record to True. The server will need to grow support for this field.

In Bug 746017, we're discussing the need to persist the name and icon of an app on the AitC server. For the first release, we'd like to do name (icons are trickier and will take up more space, so we're going to defer that for later). The main reason to persist these two is security - if the developer is allowed to change the name and icon at will without user intervention - it could be dangerous. Hence, the AitC server must be the authoritative source for these two. Would it be possible to use this bug to also add the "name" field, or should we make another one?

Sure, I've updated the patches accordingly. I'm wondering if we should relax things to accept any unknown fields without validation, at least for the first few iterations. We have a hard limit on the total size of the data so it shouldn't pose much of a problem to accept unvalidated additional fields.

(In reply to Ryan Kelly [:rfkelly] from comment #4) > I'm wondering if we should relax things to accept any unknown fields without > validation, at least for the first few iterations. We have a hard limit on > the total size of the data so it shouldn't pose much of a problem to accept > unvalidated additional fields. Yes, even though we'll want to end up with server-side validation for all the record properties at some point; in the early stages it might be more productive to relax this requirement. I anticipate even more changes in the record structure in the coming weeks as we implement the Device API on the client. +1 from me if that's easy for you to do!

Comment on attachment 619844 [details] [diff] [review] docs patch to add "name" and "deleted" fields Looks good! I'd note that deleted is a temporary field until we implement a client Device API (at which point we will track uninstalls per device), but since we're the only consumers of the API, that makes it a nit :)

Right you are, I think allowing arbitrary fields for the moment is going to cause us the least pain during development. Patch updated.

I'm a little concerned around this change. We're now going to get and return arbitrary data, outside of the bounds of the API spec? I am generally suspicious of fuzzing API contracts to this extent. If we're going to do this, we should revisit the spec and explicitly declare support for this, or we should have a timeline to remove this fuzziness before official launch.

I propose we set a timeline to remove this before official launch - this must be removed before the Device API client is ready to be checked-in. We're taking this temporary measure only to be efficient in the coming weeks as we expect a few more changes to the app record structure.

(In reply to Anant Narayanan [:anant] from comment #10) > I propose we set a timeline to remove this before official launch - this > must be removed before the Device API client is ready to be checked-in. > We're taking this temporary measure only to be efficient in the coming weeks > as we expect a few more changes to the app record structure. This is setting off all kinds of alarm bells in my head. You don't just casually change web service APIs. If you expect the app record structure to change in the upcoming weeks, we need to consider that now, before client code is checked in to m-c.

I agree... lack of versioning means we can't really just change things around as we see fit. Maybe we should just bite the bullet and add versioning?

(In reply to Gregory Szorc [:gps] from comment #11) > This is setting off all kinds of alarm bells in my head. You don't just > casually change web service APIs. If you expect the app record structure to > change in the upcoming weeks, we need to consider that now, before client > code is checked in to m-c. I'm not sure I'm following... this isn't changing the API, it is simply a temporary contract between the client and the server about ignoring one part of the API. Having an app record and all its fields validated by the server is part of the API and we're not changing that (in this bug at-least). We can't consider it now because we are not working on the Device API yet, and it doesn't affect the client code we're going to be checking in for Fx15. When we do begin work on the Device API, we *may* discover a piece of data that would fit better in the app record (and I don't know this for sure yet, just an intuition), so I want to keep that door open instead of bugging rfkelly every time we find a property that has been added, removed, or slightly changed :-) I do know for certain that we will remove the deleted flag we have right now. But that's not going to happen until Fx16. I'd prefer to keep things simple and avoid versioning, even if that means we'll have to change the API to make the app record an opaque structure that the server doesn't validate. I'd be okay with that approach, curious to learn your thoughts.

(In reply to Anant Narayanan [:anant] from comment #13) > I'd prefer to keep things simple and avoid versioning, even if that means > we'll have to change the API to make the app record an opaque structure that > the server doesn't validate. I'd be okay with that approach, curious to > learn your thoughts. This is something I proposed when we were discussing the API design [1]. IIRC, Ben was opposed to enveloping at that time, but given that we're already finding reasons for changes, I think it's an issue worth revisiting. Anant, can you touch base with Ben and see what he thinks about this? (I'm also curious about changing the "no-tombstones" principle, but that's temporary, in theory.) [1] https://mail.mozilla.org/pipermail/services-dev/2012-March/001110.html

(In reply to Mike Connor [:mconnor] from comment #14) > This is something I proposed when we were discussing the API design [1]. > IIRC, Ben was opposed to enveloping at that time, but given that we're > already finding reasons for changes, I think it's an issue worth revisiting. I *think* Ben's main opposition was to enveloping, but I we can achieve what we want even without envelopes. JSON lets us conveniently ignore properties that the server doesn't recognize, so why not do that? (i.e. keep all fields in the same object as we do now, without adding a new hierarchical element, but only define a subset of properties the server is expected to validate, or none). > Anant, can you touch base with Ben and see what he thinks about this? (I'm > also curious about changing the "no-tombstones" principle, but that's > temporary, in theory.) I'll poke Ben and try to get his thoughts on this. With respect to the tombstone, Greg rightly pointed out yesterday that the deleted property is more of a flag than a tombstone (as we reset the flag if the user re-installs the app later). As you note, it is a temporary measure, only because in v1 of AitC it is ambiguous what the user means when they uninstall an app. So for now, we just mark the app as "uninstalled" (which, in retrospect, is probably a much better name). When we have Device APIs and a fancier dashboard, we can get the precise intention of the user: remove from this device, remove from all devices, or remove the data from mozilla servers? At which point, this flag will no longer be necessary.

(In reply to Mike Connor [:mconnor] from comment #12) > I agree... lack of versioning means we can't really just change things > around as we see fit. Maybe we should just bite the bullet and add > versioning? Versioning in client or server? The server API already has the usual <URL>/<VERSION>/<USERID> prefix embedded in the endpoint_url.

In my experience, versioning data structures is a *very* expensive design. I would like to hold off on that as long as possible. I didn't realize that we were putting a version in the URL, either, I would push back against that, too, if it weren't so late in the game. (Specifically, I like stable URL resources for items, not many different URLs for the same resource.) Mike is right that I am against enveloping, especially in this case, which mostly kills the point of validation. It's fine by me if we temporarily relax some validation rules in the implementation. Would that be sufficient? How many new features are we talking about? Also, regarding the uninstalling, wouldn't that fit more under the /devices/ part of the API? That could be the longer term solution, with a short term hack for now.

(In reply to Ben Adida [:benadida] from comment #17) > I didn't realize that we were putting a version in the URL, either, > I would push back against that, too, if it weren't so late in the game. > (Specifically, I like stable URL resources for items, not many different > URLs for the same resource.) I completely agree about stable URLs, but unfortunately don't know a better way to manage backwards-incompatible API changes. Apart from "try really hard not to make any", which I am very much on board with. What I'm specifically worried about is something like this: > I do know for certain that we will remove the deleted flag we have right now. > But that's not going to happen until Fx16. If the idea here is "accept and store the deleted flag as used by Fx15, but the client will ignore it once Fx16 rolls out" then we're cool, no compatibility worries there. But if it's closer to "accept the deleted flag while we're on Fx15, but start rejecting it again when Fx16 rolls out" then we're creating a backwards-incompatibility problem. Explicit API versioning is the best way I know of dealing with this, but I'm very open to other suggestions if there is a better way to manage it. Perhaps we could do content-type negotiation to deal with the differences? I'll take my usual stance here: the server will do whatever makes the most sense for the client. My top priority right now is to unblock client development, hence this mega-relaxation of the rules while we don't have any clients in the wild. We'll need a more careful story about how to deal with API changes once the AITC client escapes into Aurora.

(In reply to Ryan Kelly [:rfkelly] from comment #18) > (In reply to Ben Adida [:benadida] from comment #17) > What I'm specifically worried about is something like this: > > > I do know for certain that we will remove the deleted flag we have right now. > > But that's not going to happen until Fx16. > > If the idea here is "accept and store the deleted flag as used by Fx15, but > the client will ignore it once Fx16 rolls out" then we're cool, no > compatibility worries there. Yes, the main problem is that with AitC we're building a new system, and no matter how careful we are while designing the API, the implementation, especially when we do it in phases like we are, will likely require some tweaks to the original design. To my mind, this is a question of *when* we're committing to a stable server API and sticking with it, it's a bootstrap problem. I don't think we're there yet. V1 is just testing the waters with a small crowd, hoping we will learn from it. So ignoring part of the spec (server validation of fields) is fine by me, as is removing validation itself. The latter makes it easier for forward compatibility, but we do lose out on some extra correctness checking (personally, I'd say a fair tradeoff). I had a bad feeling about the deleted flag to start with (Ian warned me as well). But, I want something to happen on the dashboard when the user uninstalls an app, at the same time, I want to be conservative about not deleting data from the server (which might include receipts) in this first version. So, indeed, this is a very short term stopgap, only until we implement the device API in the next iteration.

(In reply to Ryan Kelly [:rfkelly] from comment #18) > But if it's closer to "accept the deleted flag while we're on Fx15, but > start rejecting it again when Fx16 rolls out" then we're creating a > backwards-incompatibility problem. > > Explicit API versioning is the best way I know of dealing with this, but I'm > very open to other suggestions if there is a better way to manage it. > Perhaps we could do content-type negotiation to deal with the differences? > I'll take my usual stance here: the server will do whatever makes the most > sense for the client. This also brings up the larger question of how we deal with changes to the app record beyond the device API implementation (say, 6 months out from now). I think we shouldn't make the assumption that there won't be any. I think we can make it work by having the server simply ignore all app record fields right from the start (i.e. the app origin -> id URL is the only thing the server deals with). We can do this without enveloping, as well as avoid versioning in the URL. I'd like to learn more about the drawbacks of this approach! If we have to make a drastic change to the AITC origin->id mapping scheme or something like that, we can always switch to a new hostname for the "new" API. That's a clean way to solve compatibility IMO.

(In reply to Anant Narayanan [:anant] from comment #20) > I think we can make it work by having the server simply ignore all app > record fields right from the start (i.e. the app origin -> id URL is the > only thing the server deals with). We can do this without enveloping, as > well as avoid versioning in the URL. I'd like to learn more about the > drawbacks of this approach! The server must care about at least, the origin, modifiedAt and installedAt fields. The first defines the id of the record, and the others are set by the server based on its local time. Modulo enveloping of the payload, this is essentially the approach taken by Sync. The server doesn't care what you put in the record, and it's up to the clients to make sure they don't step on each other's toes. I believe Sync uses a client-side "record format version" to prevent incompatible clients from accessing the same data store. > If we have to make a drastic change to the AITC origin->id mapping scheme or > something like that, we can always switch to a new hostname for the "new" > API. That's a clean way to solve compatibility IMO. I don't see a fundamental difference between this and a version component in the path - it's still "API version number somewhere in the URI".

Per Bug 750987, the relaxaed field checking is now live on the staging machines, ready for further client testing/development.

(In reply to Anant Narayanan [:anant] from comment #20) > This also brings up the larger question of how we deal with changes to the > app record beyond the device API implementation (say, 6 months out from > now). I think we shouldn't make the assumption that there won't be any. Absolutely agreed. > I think we can make it work by having the server simply ignore all app > record fields right from the start (i.e. the app origin -> id URL is the > only thing the server deals with). We can do this without enveloping, as > well as avoid versioning in the URL. I'd like to learn more about the > drawbacks of this approach! Well, the most obvious issue is interop. If you add a property in version N, and change/remove that in N+1, now you have a data issue, because N+1 won't write that property back to the server in a way that N will understand, and then you break consistency. Even if you just consider Firefox, you're still dealing with time-to-upgrade, beta channels, etc. When you start getting multiple consumers (web dashboards, B2G phones, Firefox clients, and ideally some third party clients) you _really_ need to have a stable, predictable datastore, with a stable and enforced data format. If AITC is meant to be a canonical datastore, I don't understand why we'd want to allow clients to extend the data format arbitrarily. That really seems to shift the compatibility burden to all clients to accomodate undocumented format changes, or perhaps a Sync-style data versioned format independent of the API. I don't think either option is in line with the AITC design goals. > If we have to make a drastic change to the AITC origin->id mapping scheme or > something like that, we can always switch to a new hostname for the "new" > API. That's a clean way to solve compatibility IMO. As Ryan said, different hostname vs. version in the URL is really the same thing. Incompatibility shows up somewhere. API versioning is the least painful way I can think of. Just because we have a version doesn't mean we'll ever use it, or will _want_ to use it. But not having it means we make future changes even more complex, and feels like setting ourselves up for unnecessary pain.

Saw evidence today in aitc 1.1 Stage that this is impacting the load testing - to the tune of many 400s. :rfkelly to update loadtest to handle response due to this fix.

Loadtests were failing because they weren't sending the newly-required "name" field. Updated and pushed to master.

I'm re-opening this bug. Various out-of-band conversations indicate that we're not happy shipping the current anything-goes implementation to production, and we haven't settled on an alternative approach.

Just a reminder - this is blocking bug 744257. What is our schedule for AITC and how does that align with discussions around fixes for this bug?

Following out-of-band discussions, we've decided that: 1) the "name" and "deleted" fields are here to stay. 2) we will continue to allow records with arbitrary fields in dev and stage, but will switch this ability off in production. To this end, the attached patch adds a "storage.ignore_unknown_fields" option to the server-aitc config file. It defaults to false, we can set it to true on servers where exploratory development is taking place.

Comment on attachment 634268 [details] [diff] [review] patch to allow easy toggling of ignore_unknown_fields Looks fine. One style question: + key = "storage.ignore_unknown_fields" + self.ignore_unknown_fields = config.registry.settings.get(key, False) Any reason to do that on two lines? Just wrapping?

(In reply to Toby Elliott [:telliott] from comment #29) > + key = "storage.ignore_unknown_fields" > + self.ignore_unknown_fields = config.registry.settings.get(key, > False) > > Any reason to do that on two lines? Just wrapping? Pretty much, yeah. I couldn't find a nice place to break the line at 80 chars. Committed here: https://github.com/mozilla-services/server-aitc/commit/ed460d47d3c46273b34eb9d469be0b9d1462382f