Reverting the last patch from submission in order to further discuss it Alex
On Thu, May 21, 2015 at 11:25 AM, Michael Wood <[email protected]> wrote: > 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 < >> [email protected] <mailto:[email protected]>> 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 >> <[email protected] <mailto:[email protected]>> 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. >> >> > --------------------------------------------------------------------- > 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
-- _______________________________________________ toaster mailing list [email protected] https://lists.yoctoproject.org/listinfo/toaster
