[asterisk-dev] [Code Review] 2715: Bucket: A generic file and directory API

[Mark Michelson]

> On Aug. 19, 2013, 5:45 p.m., Mark Michelson wrote:
> > I am not a fan of having to include the uriparser as a dependency. The biggest reason is that this introduces a prerequisite for a core feature that is not actually used anywhere in Asterisk.
> > 
> > I also just don't think it's necessary. The only URI parsing that really should be necessary is to separate the scheme from the rest of the URI. Right now, when allocating a bucket or bucket file, you grab the last element of the path to use as the bucket or bucket file name. I don't see the point in doing this because:
> > 
> > 1) I don't foresee a situation where just the final path element will be useful to a scheme. I'm betting that the full path (or heck, even the full URI) will be much more useful.
> > 2) It's possible to have multiple buckets with identical names within the same scheme. This means that in order to differentiate buckets (in, say, debug messages), you'll have to use the full URI.
> > 3) The bucket->name and file->name are never referenced in the core bucket code except to set the values.
> > 
> > I say that if you want to store anything other than the scheme of a URI in a bucket or bucket file, then just store everything after the ':' or '://' in the name field so the individual scheme implementations can do what they need with the data. However, even that's not really necessary since the full URI is retrievable via ast_sorcery_object_get_id().
> 
> David Lee wrote:
>     I'm not crazy about adding the uriparser as a dependency, but I think
>     it's a good thing here.
>     
>     While parsing out the scheme from the URI is fairly simple to do on your
>     own, it quickly spins out of control once you try to parse anything else
>     out of it. But it looks deceptively easy enough that everyone tries to
>     roll their own.
>     
>     Users of the bucket API are going to need to parse out information from
>     the URI's themselves anyways. Since we would discourage roll-your-own
>     URI parsing, uriparser would become a dependency anyways.
>     
>     The package has no dependencies, and is pretty widely available; it's in
>     EPEL, Fedora 18, Ubuntu Lucid and OS X Homebrew. That satisfies my usual
>     objections to a dependency.
> 
> Mark Michelson wrote:
>     Given the current date and all, is bucket still a candidate for Asterisk 12? If not, then I'm fine with the uriparser being added as a dependency since there should also be bucket wizards added before Asterisk 13 gets created. However, if this is still an Asterisk 12 planned addition, then I think it's absolutely ridiculous to have uriparser as a dependency when it will not actually be used for anything.

Just chatted with Matt and came up with a potential compromise.

Since there may be bucket wizards added mid-stream in 12, it could work as follows:

* Go ahead and change the configure script to search for uriparser on the system; however, don't fail if uriparser is not present.
* In the bucket core, if uriparser was found, then use the uriparser in order to determine the URI scheme (have a #ifdef section). If uriparser isn't present, then you can determine the scheme from of the URI yourself with some simple string manipulation (see RFC 3986. The scheme portion is the most straightforward to determine). There's no need to mess with the URI path in the core whether uriparser was present or not.
* Once bucket wizards get introduced, they very well might actually need to do more complex URI parsing. These can have dependencies on uriparser if necessary.

This way, we're not requiring a dependency for an unused feature. Instead, the dependency is only required in order to compile a bucket wizard that requires more complex URI parsing. In a future version of Asterisk, where uri parsing is actually used in the core (possibly in the HTTP server as David mentioned on IRC), then uriparser can be escalated to be a core dependency.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2715/#review9434
-----------------------------------------------------------


On Aug. 19, 2013, 3:54 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2715/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2013, 3:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Bucket is a generic file and directory API implemented as a thin wrapper over sorcery. It decouples the storage of files from the backend and provides the ability to set metadata on file themselves. What it means to have files in a "bucket" and details about the files themselves is left up to the user of the bucket API. It enforces no restrictions. Identification of things is done using URIs, which include a scheme. Modules implement specific schemes (local, for example) and take care of the implementation specific aspect of it. The ability to get bucket and file information in a JSON format is provided, as is the ability to copy and move files.
> 
> Stuff to focus on:
> 
> 1. Is the API sane and complete?
> 2. Do the unit tests cover everything?
> 3. What do we want to do for URI parsing - I wrote a basic extremely optimistic one
> 
> 
> Diffs
> -----
> 
>   /trunk/main/bucket.c PRE-CREATION 
>   /trunk/main/asterisk.c 396929 
>   /trunk/main/Makefile 396929 
>   /trunk/include/asterisk/config_options.h 396929 
>   /trunk/include/asterisk/bucket.h PRE-CREATION 
>   /trunk/include/asterisk/autoconfig.h.in 396929 
>   /trunk/configure.ac 396929 
>   /trunk/configure UNKNOWN 
>   /trunk/build_tools/menuselect-deps.in 396929 
>   /trunk/main/config_options.c 396929 
>   /trunk/main/sorcery.c 396929 
>   /trunk/makeopts.in 396929 
>   /trunk/tests/test_bucket.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2715/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests, confirmed functional. Tweaked this to purposely break and confirmed they broke.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130823/95245187/attachment.htm>