[Toaster] [review-request] adamian/20150515_fix_table_header
Michael Wood
michael.g.wood at intel.com
Thu May 21 03:25:00 PDT 2015
Hi,
Conflating the web pages and the APIs into a single URL pattern/schema
just doesn't make sense to me because:
- We will have pages calling themselves with a different parameter (e.g.
the tables pages)
- This is not how REST frameworks are implemented in any other
application I've seen before
- In the future we may want to version the name-space e.g.
/api/1.3/projects/
- Keeping the API self contained allows for greater future flexibility
because it de-couples them from the page structure.
- REST should be as self documenting as possible. Having to know which
URLs support JSON responses isn't helpful.
- The tree of objects in a web page doesn't not have a 1:1 mapping with
the API so they functionally different, in only a few cases do we have 1
page = 1 set of data.
- The duplication is just having a url entry and a view that has a
query+json response (you could easily do this as subclassed view,
ToasterJsonView.as_view(QuerySet)) really that's not a lot. If you use
pre-defined querysets that's even easier, the abstraction can make the
duplication negligible.
- Soon the API calls are going to be the main way in which toaster get's
it's data and the template mechanism is going to be used very lightly.
There are still a few other issues in the patch, but I think we need to
get this sorted first as it affects everything else.
I'd be much happier to have along these lines
/api/projects/
/api/project/1/
/api/project/1/?bitbake="core-image-minimal"
.
.
Current ToasterTable handles this already (could be tweaked to have a
different modes):
/api/project/1/compatible/machines/
/api/project/1/compatible/layers/
/api/project/1/compatible/recipes/
.
.
To add (though we currently only have a use case for layer at the moment):
/api/project/1/compatible/layer/3
/api/project/1/compatible/machine/3
/api/project/1/compatible/recipe/3
.
.
Michael
On 21/05/15 11:12, Damian, Alexandru wrote:
> Taken for submission,
>
> Cheers,
> Alex
>
> On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru
> <alexandru.damian at intel.com <mailto:alexandru.damian at intel.com>> wrote:
>
> Hi,
>
> I've pushed a new version of the patch for review, on the same branch.
>
>
> All of Belen's remarks have been amended, except
>
> "* If I select a project, then refresh the page, the page has
> forgotten
> about the project I just selected. It would be nice to remember
> the last
> project I chose.
> "
>
> This is because, except for "All Projects" and "All Builds" pages,
> we already have a default Project - the project under which we
> currently are. We need a design decision to see which option has
> more priority - the previous selection of the user, or the project
> under which the page is displayed.
>
>
> I've added a couple of tests in Django for toastergui (separate
> patches) to validate that the /projects endpoint returns valid
> data, and exemplify the usage.
>
> I have more remarks below,
>
> Cheers,
> Alex
>
>
> On Tue, May 19, 2015 at 5:31 PM, Michael Wood
> <michael.g.wood at intel.com <mailto:michael.g.wood at intel.com>> wrote:
>
> Hi,
>
> Review below.
>
> -
> - libtoaster.makeTypeahead(newBuildProjectInput, { type :
> "projects" }, function(item){
> + libtoaster.makeTypeahead(newBuildProjectInput,
> libtoaster.ctx.projectsUrl, { type : "projects", format :
> "json" }, function(item){
> /* successfully selected a project */
> newBuildProjectSaveBtn.removeAttr("disabled");
> selectedProject = item;
> +
> +
> })
>
>
> I understand the having the xhrUrl as a new parameter, but it
> doesn't make any sense to me to have "type" and "format", no
> one should expect setting the type to json as an argument to
> some magic urls in the request would then return something to
> consume for an API. How are you supposed to know which urls
> support JSON responses? Really wouldn't be happy with that.
>
>
> I removed the "type" parameter in this particular call, since it
> made no sense - the type of the object is specified by the URL in
> REST fashion.
>
> All the REST endpoints will support JSON responses, and it will be
> easily done by decorating them with the "template_renderer"
> decorator. This will be done in a subsequent patch.
>
> I do not follow the remark about "magic" URLs. All URLs are
> documented and follow-able in REST fashion:
>
> /projects/ - list all projects, or create new project (by POST)
> /project/ID/ - list project by id, or update project data (by POST)
> etc...
>
> the idea is that the URLs called without the *format* parameter
> will return text/html, and the same URLs called with the
> *format=json* parameter will return application/json, in a
> predictible fashion
>
>
> If you're wanting to avoid having a query for the projects
> html page and another copy of it for the API response why not
> use a defined Queryset? the django QuerySet managers are
> exactly for this.
>
>
> What I want to avoid is view code replication, at all levels.
> e.g. the computation for compatible layer versions are done
> separately two times, with nearly identical code - when computing
> suggestions for returning JSON files, and when displaying project
> layers table. There is no reason for this code duplication, and
> for different API entry points.
>
> This isn't just about selecting objects from the database, but
> also about processing commands from the user - I think we should
> not have dual views for whenever we need to render HTML or JSON -
> we only need to be preoccupied with computing the correct data,
> and let the presentation layer - in this case the
> template_renderer decorator - worry about presenting the data in a
> suitable format.
>
>
>
> + /* we set the effective context of the page to the
> currently selected project */
> + /* TBD: do we override even if we already have a
> context project ?? */
> + /* TODO: replace global library context with references
> to the "selected" project */
> + libtoaster.ctx.projectPageUrl =
> selectedProject.projectPageUrl;
> + libtoaster.ctx.xhrProjectEditUrl =
> selectedProject.xhrProjectEditUrl;
> + libtoaster.ctx.projectName = selectedProject.name;
>
>
> What happens if something else accesses those afterwards? If
> you select a project then close the popup you've potentially
> broken any other "user" of that context data.
> Ideally those properties are read-only.
>
>
> They were already modifiable by building URLs by appending
> specific IDs to base_ URLs - in this sense, the change only makes
> evident that the URLs are not static at all. Furthermore, I agree
> that we shouldn't build URLs by hand in JS code, so getting the
> URLs in complete form from the backend is cleaner, IMHO.
>
> Since these URLs live in the context of the page, they always
> operate on the current project set in the context, no matter the
> caller. I think this is in accordance with the current design of
> the "New Build" button, which sets a current Project in page
> context for follow-up actions to be taken on it - it would be
> strange if any pieces of code would want to operate on another
> project without reflecting this to the user.
>
>
>
> +class RedirectException(Exception):
> + def __init__(self, view, g, mandatory_parameters, *args,
> **kwargs):
> + super(RedirectException, self).__init__()
> + self.view = view
> + self.g = g
> + self.mandatory_parameters = mandatory_parameters
> + self.oargs = args
> + self.okwargs = kwargs
> +
> + def get_redirect_response(self):
> + return _redirect_parameters(self.view, self.g,
> self.mandatory_parameters, self.oargs, self.okwargs)
> +
>
> Using HTTP redirects are a real pain because they get cached
> by the browser and are very difficult to invalidate.
>
>
> The redirects are now made temporary.
>
> I think it's nicer to provide the user with defaults for options
> they haven't selected, and I know no better way to reflect that to
> the user but by issueing a redirect.
>
>
>
> - def projects(request):
> - template="projects.html"
> + def template_renderer(template):
> + def func_wrapper(view):
> + def returned_wrapper(request, *args, **kwargs):
> + try:
> + context = view(request, *args, **kwargs)
> + except RedirectException as e:
> + return e.get_redirect_response()
> +
> + if request.GET.get('format', None) == 'json':
> + # objects is a special keyword - it's a
> Page, but we need the actual objects here
> + # in XHR, the objects come in the "list"
> property
> + if "objects" in context:
> + context["list"] =
> context["objects"].object_list
> + del context["objects"]
> +
> + # convert separately the values that are
> JSON-serializable
> + json_safe = {}
> + for k in context:
> + try:
> + jsonfilter(context[k])
> + json_safe[k] = context[k]
> + except TypeError as te:
> + # hackity-hacky: we serialize the
> structure using a default serializer handler, then load
> + # it from JSON so it can be
> re-serialized later in the proper context
>
>
> # hackity-hacky !!
>
> We're doing re-factoring to remove hacks and awkward code,
> let's not be adding them back in at the same rate.
>
>
> I've changed this to a cleaner version with no hacks.
>
>
>
> Michael
>
>
>
>
> On 19/05/15 16:27, Damian, Alexandru wrote:
>
> Hi,
>
> Can you please review this branch ? It brings in an
> important piece of refactoring to move Toaster towards the
> REST-style API that will enable a cleaner design and
> support for interconnection with other systems.
>
> Getting into details, the "New build" button in all the
> pages, bar the project page, now fetches data using the
> projects API, which is just the "all projects" page in
> machine-readable format. This cleans up a bit the
> overloading of the xhr_datatypeahead; after the API
> refactoring, all xhr_ calls should be completely replaced
> with proper calls to the REST endpoints.
>
> The code is here:
> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>
> Incidentally, this patch fixes the "new build" breakage by
> the first round of URL refactoring, and the "construction"
> of URLs on the client side.
>
> Can you please review and comment ?
>
> Cheers,
> Alex
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential
> material for
> the sole use of the intended recipient(s). Any review or
> distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential
> material for
> the sole use of the intended recipient(s). Any review or
> distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>
>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
More information about the toaster
mailing list