Bugzilla – Full Text Bug Listing
| Summary: | [commands] command syntax in page URL's | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Susan McCourt <susan> |
| Component: | Client | Assignee: | Susan McCourt <susan> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | john.arthorne, malgorzata.tomczyk, mamacdon, simon_kaegi, susan, Szymon.Brandys, tomasz.zarna |
| Version: | unspecified | ||
| Target Milestone: | 0.4 M1 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 334196, 344205, 359080 | ||
|
Description
Susan McCourt
We have to be very careful about introducing a syntax for invoking commands via HTTP GET. The purpose of GET is simply to obtain a resource representation. If you invoke it multiple times you should get the same result. It should never modify resources. The reasons are nicely captured in the HTTP spec section 9.1. http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html Now in the case of "Goto line" this is safe because it is just a position in a resource. This is supposed to be done with an anchor tag rather than a query (foo.js#line_25 or similar). But if we allowed a command like "Delete line 25" in the URL this is no longer a safe thing to do (using HTTP's definition of "safe"). (In reply to comment #1) > We have to be very careful about introducing a syntax for invoking commands via > HTTP GET. The purpose of GET is simply to obtain a resource representation. makes sense. The use cases we have now involve navigation of the resource in question, but I agree we need to be careful in defining how this works and not opening it up for random commands. Another use case that McQ suggested is being able to initiate a Git clone from a single URL (bug 359080). One thing that came to mind here is that the user might click a link by accident, not realize what it is doing, etc. So I think in general the most we could do is open the page with a popup that asks for confirmation before actually proceeding with any change. From that perspective it is a GET on a resource representing the page with the popup open - it doesn't actually modify anything so the GET is perfectly safe. Hitting F5 on the page will not cause something to be repeated. Another case: We plug in some Git actions to File Navigator e.g. Git Log or Git Status that open Git pages. We can't add actions that have to collect data from the user though. So there is no way to init a repo in a folder from File Navigator. We need to open Git Clones and then use Git Init. And Git Init opens a dialog asking where to init a repo. If we had such command URLs, we could add the Git Init action to File Navigator. It would open Git Clones page and start Git Init immediately. I chatted with Susan about it and there is another way to fix "Git Init on File Navigator" issue. It is bug 358767. I've pushed some initial support for this feature in the command service, with an example implementation in the "New Folder" command in the navigator. (The example will be removed at some point, because it's not that interesting to create a new folder by URL). The new concept is "URLBinding" and it's treated similar to a KeyBinding. When you contribute a command, you can define a URL binding for the command. Specifically, you define the token that represents the command in the URL, and the name of the parameter that is after the = sign. So New Folder sets up a token of "newFolder" and a parameter name of "name" and that means that a URL containing newFolder=Foo as a query parameter will execute the new folder command with "Foo" as the value of the parameter called "name." The new folder is not created, but rather the slideout comes out proposing the name. I'm trying to keep the URL syntax simple, so this assumes that only parameter is expected in the URL. If we needed to collect multiple parameters, we would need a more complex syntax like: newFolder.name=Foo&newFolder.someOtherParameter=value The next example to try is git clone. This case is more complex because the current dialog has advanced options. We may not want to expose these advanced options when the command is invoked by URL, but either way, I need to work on a more general scheme that lets the command say which parameters are required, which are optional, and when the command is invoked by URL, we could possibly skip the optional ones and provide a button to invoke the full dialog if needed. Need to think about this some. Susan are these: 1)query parameters on the task 2)query parameters on the resource 3)additional fragment parameters My concerns is that: - approach (1) will impact caching - approach (2) implies either Orion or the relevant plugin knows how to break apart a url I think we need to be pretty careful here. I haven't given a ton of thinking on this but was thinking approach (3) was probably the best route as it's information used by the task. Approach (1) might also be ok but again breaks caching. These are additional fragment (post hash) parameters. For example, in the example, invoking "new folder" on the navigator is: http://127.0.0.2:8080/navigate/table.html#/file/c/?depth=1&newFolder=foo Similar to today's http://127.0.0.2:8080/edit/edit.html#/file/c/bundles/org.eclipse.orion.client.core/web/index.js?line=39 (I would even like to change the implementation of position parsing on the editor to use the command approach, so that "line=39" isn't special, it's just the editor contributing a "line" URL binding for "goto line") I think it makes sense as part of the fragment. Really this is identifying a particular position/state of the page rather than a different representation. I.e., it is "the editor page scrolled to line 50", or "repositories page with slideout visible" The only problem is the client code will have to parse and pull apart the fragment to get the information it wants. Are you thinking of still just sending the entire fragment to the server to fetch the resource? I.e., given: http://127.0.0.2:8080/navigate/table.html#/file/c/?depth=1&newFolder=foo Will you send the entire fragment to the file service: GET /file/c/?depth=1&newFolder=foo And assume the service will ignore any parameters it doesn't understand? The only alternative is the client side knows which parameters it is responsible for and strips them off, but adds a lot of complexity on the client code... (In reply to comment #8) > Will you send the entire fragment to the file service: > > GET /file/c/?depth=1&newFolder=foo > > And assume the service will ignore any parameters it doesn't understand? Yes, this is what my first implementation does. Out of curiosity I had tried GET /file/c/?newFolder=foo&depth=1 to see what the server did, and it worked. So I thought this was reasonable for the first pass. > The only alternative is the client side knows which parameters it is responsible > for and strips them off, but adds a lot of complexity on the client code... Right. Currently we have different bits of client code that know different things about fragment format, so each bit would have to do that, which we'd have to have some central/known place to modify the fragment as you parsed it. I wanted to wait for a problem to appear before moving in this direction. Another thing that may come up that I'm not dealing with initially...order dependence on commands. I can imagine that we might expect http://127.0.0.2:8080/edit/edit.html#/file/c/someFile.js?line=39&find=dojo (find the first occurrence of "dojo" starting at line 39) to produce a different position in the editor than http://127.0.0.2:8080/edit/edit.html#/file/c/someFile.js?find=dojo&line=39 (find dojo and then goto line 39) Right now the parsing code will favor the url binding that was registered first rather than look them up in order, but the code is not optimized at all right now. The use of query parameters on the resource (e.g. the url in the fragment) is very fragile because in some cases the resource we're acting on might _really_ use query parameters to find it. (e.g. http://myhost/file?myfiles/myscript.js) I was hoping what we would have is something describable by a URI template so something like: http://orion.eclipse.org/naviaget/table.html{#resource,keys*} --> http://orion.eclipse.org/naviaget/table.html#/file/c/?depth=1,newFolder=foo,a=7,b=1 The only problem here is that "," takes on a bit of a special role as a separator however the benefit is we can describe the supported syntax in terms of a URI template. Here's a link to the spec for easy access -- http://tools.ietf.org/html/draft-gregorio-uritemplate-07 (In reply to comment #11) > http://orion.eclipse.org/naviaget/table.html{#resource,keys*} > > --> > > http://orion.eclipse.org/naviaget/table.html#/file/c/?depth=1,newFolder=foo,a=7,b=1 > > > The only problem here is that "," takes on a bit of a special role as a > separator however the benefit is we can describe the supported syntax in terms > of a URI template. That does sound more orderly. The extra separator character doesn't bother me that much, though we have had issues before where special characters in the file name got parsed incorrectly. I will defer to you and John to settle on what makes the most sense and adjust my parsing code accordingly. I've released code that allows a git clone to be triggered with a URL binding of "cloneGitRepository" so that you can now use a link like this: http://127.0.0.2:8080/git/git-clone.html#/workspace/A?cloneGitRepository=someURL I feel there is enough support in the command framework to close this bug. There is still some discussion here about the URL syntax and separating the resource part of the fragment from the client-side part. I created bug 363960 to track that discussion. I still have a lot of code cleanup and documentation for how this works to do, but there are other bugs covering that. (In reply to comment #14) > I've released code that allows a git clone to be triggered with a URL binding > of "cloneGitRepository" so that you can now use a link like this: > > http://127.0.0.2:8080/git/git-clone.html#/workspace/A?cloneGitRepository=someURL Very cool! Currently I have to click "More", and then click Ok in the clone dialog. Could there also be a "Clone" button directly in the slideout to save the extra step? I imagine the 95% case is not caring about configuring the directory it lives in.. maybe I'm over-streamlining here though. (In reply to comment #14) > I've released code that allows a git clone to be triggered with a URL binding > of "cloneGitRepository" so that you can now use a link like this: > > http://127.0.0.2:8080/git/git-clone.html#/workspace/A?cloneGitRepository=someURL Note, I tried to use this to add a "Fork me on Orion" link on our wiki page. This fails because the workspace id will be different for each user. I entered a new bug for this, See bug 363977. (In reply to comment #15) > Very cool! Currently I have to click "More", and then click Ok in the clone > dialog. Could there also be a "Clone" button directly in the slideout to save > the extra step? I imagine the 95% case is not caring about configuring the > directory it lives in.. maybe I'm over-streamlining here though. You are correct, the common case should be just the URL. I was worried about the UI here. You can hit enter to clone. It seems more obvious in something like "new folder" when there's no 'More' button begging you to click it. I've been wondering if we need an "OK" button next to the close/cancel icon to make it clearer. Also we should style the "More" button to be more subtle (smaller font, etc.) Opened bug 364041 for this. |