Amazon S3 data provider

[dylan]

It would be cool if we had an Amazon S3 data provider.

[peller]

Brian, I'm removing the 0.9 milestone on this. Please assign/dispose of this as you see fit

[skinner]

An Amazon S3 datastore sounds great. Unfortunately, I don't think either jared or I are likely to get around to this in the months to come. We're short on volunteer hours for dojo.data work. But I'd happily review a contribution, if you can send one in.

[ttrenka]

Removing the milestone.

[dylan]

I'd like to see this implemented by 1.2.

[kriszyp]

Working on it...

[kriszyp]

Created the following to support Amazon S3: ProxiedPath?.js - A variation of the "PATH" envelope to put the url in a parameter for a proxy s3/** - PHP-based proxy to forward requests to the Amazon S3 service S3JsonRestStore - An extension of JsonRestStore? for interfacing with S3

To connect to S3 with our RPC service:

dojo.require("dojox.rpc.Service"); dojo.require("dojox.rpc.ProxiedPath?"); var s3Buckets = new dojox.rpc.Service({

target:"http://s3.amazonaws.com/", proxyUrl:"../s3/proxy.php", transport:"REST", envelope:"PROXIED-PATH", contentType:"application/json", services:{

kris:{

target:"kris", parameters:[{type:"string"}]

}

}

});

To use the S3JsonRestStore:

dojo.require("dojox.data.S3JsonRestStore"); s3Store = new dojox.data.S3JsonRestStore({service:s3Buckets.kris});

Hopefully this will fulfill the primary use cases for S3. It would still be nice to have RestStore? that just worked with the S3 data values as opaque strings. This wouldn't be as functional as the JsonRestStore?, but could accommodate the different formats that S3 allows.

[kriszyp]

Argh, my patch was 20KB over the size limit of 262KB. You can grab the patch from here: http://persvr.org/test/s3.diff Someone may want to check to see if the included files are all legal for inclusion in Dojo, I used examples from S3, and some PHP library files. We could certainly force users to download the PHP libraries.

[peller]

we cannot commit Zend code (or any code not originating from you) without a CLA and their consent, and I think we should generally avoid checking in application code under revision control elsewhere, especially if it would bloat the library size. We have other modules which require hefty server code to operate, like dojox.offline. Perhaps we should look into installing running demos on the site somewhere.

[kriszyp]

No problem, the PHP proxy script is trivial, I can easily rewrite. And I will just include directions for downloading/retrieving the dependencies. It will probably be about 3KB.

[kriszyp]

All right, I just uploaded a patch that should be clean.

[dylan]

Jared, can we get a code review on this?

Kris, I've noticed some code blocks commented out in a few places. Should those be removed?

[kriszyp]

Just FYI, this is dependent on http://trac.dojotoolkit.org/ticket/5987.

[jaredj]

Will review, yeah.

[jaredj]

Okay, did a quickish review and found a few issues that need to be addressed. (there may be more) Here is what I found:

Problem #1: Improper definition of loadItem. loadItem is an asynchronous call and takes callbacks, yet your definition in ServiceStore? is:

loadItem: function(item)

It should be:

loadItem: function(/* object */ keywordArgs)

// summary: // Given an item, this method loads the item so that a subsequent call // to store.isItemLoaded(item) will return true. If a call to // isItemLoaded() returns true before loadItem() is even called, // then loadItem() need not do any work at all and will not even invoke // the callback handlers. So, before invoking this method, check that // the item has not already been loaded. // keywordArgs: // An anonymous object that defines the item to load and callbacks to invoke when the // load has completed. The format of the object is as follows: // { // item: object, // onItem: Function, // onError: Function, // scope: object // } // The *item* parameter. // The item parameter is an object that represents the item in question that should be // contained by the store. This attribute is required.

// The *onItem* parameter. // Function(item) // The onItem parameter is the callback to invoke when the item has been loaded. It takes only one // parameter, the fully loaded item. // // The *onError* parameter. // Function(error) // The onError parameter is the callback to invoke when the item load encountered an error. It takes only one // parameter, the error object // // The *scope* parameter. // If a scope object is provided, all of the callback functions (onItem, // onError, etc) will be invoked in the context of the scope object. // In the body of the callback function, the value of the "this" // keyword will be the scope object. If no scope object is provided, // the callback functions will be called in the context of dojo.global(). // For example, onItem.call(scope, item, request) vs. // onItem.call(dojo.global(), item, request)

Problem #2: Potentially improper paging behavior. Your fetch call in ServiceStore? states:

// onBegin: /* function */ // called before any results are returned. Parameters // will be the count and the original fetch request //

Which does not follow the spec definition of the value being:

// The *onBegin* parameter.

// function(size, request); // If an onBegin callback function is provided, the callback function // will be called just once, before the first onItem callback is called. // The onBegin callback function will be passed two arguments, the // the total number of items identified and the Request object. If the total number is // unknown, then size will be -1. Note that size is not necessarily the size of the // collection of items returned from the query, as the request may have specified to return only a // subset of the total set of items through the use of the start and count parameters. //

By that definition, the parameter passed to onBegin is not necessarily count. As you could have 100 matches, but say only return 10, starting at index 20. With onBegin getting just the value of count (instead of total identified), widgets that rely on onBegin to guestimate the size of the data view (such as grid), will not function properly. Grid will not be able to dynamically scroll as they do currently.

Problem 3: No store unit tests. The stores ought to have the basic set of unit tests that try each API and also verify that the stores provide implementations of all the functions of each API defined in getFeatures. All the other stores do those sort of tests, the _functionConformance tests It would have caught problem #5 for you.

Problem #4: Dylan already noted this. Remove code that is commented out. If it’s not used, no reason to leave it in the file. It just bloats the download time. Problem #5: Missing Read API: close(). Close is a function that is by spec supposed to be defined on the store, even if it doesn’t do anything (for API compatibility).

Problem #6: containsValue of ServiceStore? may not work correctly. The definition of it shows it just calling getValue and testing if the return is equal to value. The question is … for multi-valued attributes …. This test won’t work right. Should it not be calling getValues() and iterating over all values and testing to see if any equals? More like:

return dojo.some(this.getValues(item, attribute), function(possibleValue){

if(possibleValue == value){

return true;

}

});

If it’s not possible to have actual multi-valued attributes, then the getValue test is adequate. However, if it is possible to have multivalued attributes, then the current containsValue is not adequate.

Problem #7: isDirty(item) is not correctly implemented in ServiceStore?. You require an item to be passed and you test only if that item is dirty. The spec states:

isDirty: function(/* item? */ item)

// summary: // Given an item, isDirty() returns true if the item has been modified // since the last save(). If isDirty() is called with no *item* argument, // then this method returns true if any item has been modified since // the last save(). //

Problem #8: Odd getValue definition with a callback. I understand why you put that there, but I’m not sure it’s necessary if you properly implemented the loadItem function. loadItem could handle resolving all the properties if it’s only partially loaded and still give you async behavior. Granted, it would need to resolve all of them, instead of on a case by case basis. But that may not be efficient enough for your intentions. Not sure anything needs to be done here, just noting.

Problem #9: Error in getIdentity definition in JsonRestStore?. You specify: getIdentity : function(). So, it’s missing the parameter ‘item’, which you then use in the function. This will fail. As noted by problem #3. No unit tests. Unit tests of the apis would have caught this.

Problem #10. newItem definition of parentInfo incorrect according to spec. It is missing the attribute name on the parent to assign the item to. You state: // parentInfo: // An optional javascript object defining what item is the parent of this item (in a hierarchical store. Not all stores do hierarchical items), // and what attribute of that parent to assign the new item to. If this is present, and the attribute specified // is a multi-valued attribute, it will append this item into the array of values for that attribute. The structure // of the object is as follows: // { // parent: someItem, // }

Spec states:

// parentInfo: // An optional javascript object defining what item is the parent of this item (in a hierarchical store. Not all stores do hierarchical items), // and what attribute of that parent to assign the new item to. If this is present, and the attribute specified // is a multi-valued attribute, it will append this item into the array of values for that attribute. The structure // of the object is as follows: // { // parent: someItem, // attribute: "attribute-name-string" // } //

[kriszyp]

S3 Store

[dylan]

fixed in #13952, thanks Kris!

[dylan]

Err, fixed in [13952]

[kriszyp]

Also, Jared, I appreciate the review/feedback. I believe all these issues should be fixed now, except that I am currently working on unit tests. I will try to get those checked in shortly.

[jaredj]

Excellent. Thank you. The UT really helps avoid regressions and the like. It also helps have a place to experiment and test out the store in general, making it a bit easier for other poeple (such as me), to look at issues with it faster.