Tags:
create new tag
, view all tags
Moved from Bugs:Item2614 for public debate and inclusion in WhatIsIn04x01

When one uses checkTopicEditLock to check whether a topic can be edited, the resulting oops dialog, in case there is a conflict, moves one on to the Edit mode, when the user chooses "edit anyway". However, this check might have been invoked from other scripts (e.g., a plugin).

It must be possible to pass the script name to this function, so that the oops continues were one left off, rather than dumping one into the edit mode.

This was not a problem in the past, when we had the lock out editing model. However, now we reasonably can continue editing in parallel, so we also need to make sure that all possible means of editing are covered.... (Probably worth double checking whether attaching while somebody else edits continues across this oops...).

(See Bugs:Item2614 for proposed code)

-- ThomasWeigert


This strikes me as being a bit like poor-mans exception handling.

I didn't want to change the checkTopicEditLock API, but if I had I would have made it throw the appropriate oops exception. I would recommend ignoring the return result and throwing your own oops exception, with the correct script name filled in. That also doesn't require an API change, so I'm discarding this (I have added an enhancement request to formally add the exception types to the APIs available to extension authors).

-- CrawfordCurrie

Crawford, I don't agree. The API for checkTopicEditLock is plain wrong. No need to perpetuate that.

If the right strategy is to throw one's own exception, than that should be done everywhere, and this part should be removed from checkTopicEditLock, so people are not led astray. I resurrected this report, so that there can be more debate.

-- ThomasWeigert

Thomas has something here.

Just take a page with an EDITTABLE

Try editing the page normally and back out of it instead of cancel.

Now click the edit button on the Edit table. Yes. An oops. And then you end in normal edit mode instead of edit table mode.

There are other examples where you go via oops to normal edit where you should not.

Something is needed.

-- KennethLavrsen - 15 Jul 2006

Any API that returns an oops dialog is bad news. But as you know, we run into a stone wall whenever we even hint at changing the API, broken or not, so I would suggest a new API function that does what you want. As you know, checkTopicEditLock simply uses getLease, so the sensible thing is to expose getLease and setLease through the Func API.

getLease( $web, $topic ) -> \%lease

  • $web - web for topic
  • $topic - topic
Whenever the edit script edits a topic, it takes out a lease on behalf of the user doing the editing. A lease is not a lock; it doesn't stop anyone else from editing the topic, it is just a flag that the topic is currently being edited. Only one lease can be active on a topic at a time. Leases are used to warn if another user is already editing a topic.

If there is no lease on the topic, this function will return undef. If there is a lease, it will return a reference to a hash containing user, taken and expires). user is a wikiname, and taken and expires are absolute times in seconds.

setLease( $web, $topic, $length )

Take out an lease on the given topic for this user for $length seconds from now. If $length is <= 0, it will clear any existing lease.

See getLease for more details about Leases.

I think this is widely enough used to justify going into 4.1

Please re-open Bugs:Item2614 when you are ready. I attached a patch there.

-- CrawfordCurrie - 24 Sep 2006

Crawford, please expand on the following:

  • Why is returning an oops bad news (in particular, since you recommended that earlier up the page), and what should people do instead. I think people often follow examples (at least I do), and there are many such examples in the core code.
  • What do you mean by "when you are ready"?
  • How does the lease API solve this problem?

P.S. Reopened Bugs:Item2614 as a reminder of this problem....

-- ThomasWeigert - 24 Sep 2006

Returning an URL is bad, because it is only one of a range of possible reactions to a lock. Methods in the API should not assume you are going to redirect, and you certainly shouldn't be forced to parse an oops URL to find who has the lock and when it expires.

"When you are ready" means "When you have read this and agree with me, or we have fought it out" wink

The lease API allows you to make your own decisions about the target of the redirect instead of imposing on you. For example, I might do this in a command line script:

my $lease = TWiki::Func::getLease($web, $topic);
if ($lease && $lease->{expires} > time()) {
   print "$lease->{user} has an active lease on $web/$topic; do you want to ignore it and firtle the topic anyway? [y/n]";
   ...
or this (in CGI or command line):
# lease the topic for 100000 seconds, so that editors are warned that we are doing evil things to it
TWiki::Func::setLease($web, $topic, time() + 100000);
... do evil things...
TWiki::Func::setLease($web, $topic, 0); # clear the lease
or this (in a CGI script):
my $lease = TWiki::Func::getLease($web, $topic);
if ($lease && $lease->{expires} > time()) {
   # Active lease; send them to another URL (*not* an oops URL)
   print TWiki::Func::redirectCgiQuery( $anotherURL, 1);
   exit 0
}

-- CrawfordCurrie - 24 Sep 2006

Let me see if I understood. You are suggesting that we

  • Get rid of the TWiki::Func::checkTopicEditLock API, and
  • replace it with the exposed TWiki::Func::getLease API
so that the Plugin writers can do their own response to somebody holding a lease?

You did not really say get rid of TWiki::Func::checkTopicEditLock but I guess what would the point be of keeping it around? On the other hand, if we want to make things easier in the API for plugin writers, we might as well package the whole thing up, but allow them to send their own return point into TWiki::Func::checkTopicEditLock (my original proposal).

I think we should do one or the other, but not both?

-- ThomasWeigert - 24 Sep 2006

We have to keep checkTopicEditLock around because of the compatibility millstone. It is so dysfunctional that I propose to simply undocument it and replace with get/set lease. I certainly do not want to change the spec of it, though.

-- CrawfordCurrie - 30 Sep 2006

Did a quick search on SVN for all plugins using checkTopicEditLock.

Since not all plugins are on SVN there could be more.

If someone want to propose deprecating the API function then fixing at least the plugins on SVN that uses it should be a condition to do that. Otherwise leave it as is. But since half the plugins are Thomas' plugins, one plugin has a bug which is indirectly caused by the old API and one is not released yet - fixing the plugins and deprecating is within reasonable reach.

But if the two of you can agree on exposing getLease and setLease through the Func API and noone else disagree then we have a consensus. And as long as checkTopicEditLock is not touched I see no need to discuss this part of the proposal at a release meeting.

-- KennethLavrsen - 30 Sep 2006

Is anyone going to drive the implementation of getLease and setLease or do we defer it to 4.2?

-- KennethLavrsen - 16 Oct 2006

I am putting this in PARK since noone picks it up.

-- KennethLavrsen - 23 Oct 2006

I am unclear what needs to be done. The current situation is not acceptable, as the new model has screwed up the behavior of all non-standard edit behaviors. This MUST be fixed. I think the problem is that we are unclear how far we can go here. Crawford points out that a new mode of protection is required in the API that goes beyond checkTopicEditLock. Ken points out that there are no plugins that use this in the first place (which, by the way, is a sign that most plugins are not careful about checking for conflicts with other users; this was not a problem in the past, where one locks the page, but this is a big problem now.)

So let's move forward here....

-- ThomasWeigert - 23 Oct 2006

With respect to adding getLease and setLease all needed is someone saying: "I am willing to write the code, check it in to SVN, and fix any problems that comes around". Noone has voiced oppinions against getLease and setLease. So that is a done deal that just waits for the volunteer.

With respect to checkTopicEditLock concern has been raised, but I evaluate that if someone is willing to fix the 6 plugins so they no longer use the function, it can go on the deprecation list.

If you pick it up Thomas - at least getLease and setLease - then this can be a done deal in notime.

-- KennethLavrsen - 24 Oct 2006

I have no problem with adjusting the plugins that leverage checkTopicEditLock. However, I think that CC is the right person for working out the proper real solution.

I have no idea whether getLease and setLease are the right thing... we just need to be sure that when a plugin is trying to write to a topic it is not thrown into the normal edit mode when the user says "edit anyways".

-- ThomasWeigert - 24 Oct 2006

Check out Bugs:Item2614. It is CC that has a patch ready to be checked in.

And I understand his statements as being with you. The only missing steps are.

  • Changing the core as proposed in the patch and above
  • Fixing the few plugins because this eliminates any religious discussions about deprecating API.

So I see nothing but green lights now.

-- KennethLavrsen - 24 Oct 2006

OK. I will apply CC's patch, rewrite a plugin to take advantage of this, and report back...

-- ThomasWeigert - 25 Oct 2006

We also need an oops dialog that we can jump to and back if there is a lease already taken...

-- ThomasWeigert - 26 Oct 2006

I rewrote the locking code in EditTablePlugin to mirror what is done in TWiki::UI::Edit::edit but it somehow does not quite work. The dialog comes up but not quite, I have to press the reload button, then the oops is built, but on "edit anyway" it does not fully instantiate the template...

Not sure what is going on here...

-- ThomasWeigert - 26 Oct 2006

I think there is a bug in the oops screen upon lease right now... Bugs:Item3060

  • Fixed -- TW

-- ThomasWeigert - 26 Oct 2006

Going back to discussing this feature... When rewriting the plugin code as described above, I really ended up just doing the same thing as the edit script is doing... trying to get a lease, checking whether this is a different user, checking whether the lease has expired, and then throwing the oops.

My assumption is that the point of checkTopicEditLock (ignoring the incorrect name) was to abstract from that detail and give a simple function to the plugin authors that did all that?

If that assumption is true, then making setLease and getLease available does not really help, but just leads to code duplication again.

We need to understand whether there are likely to be scenarios where a user wants something different from the behavior currently in the edit script. Given that very few plugins actually bother to even ensure the topic is not being written to by multiple users I would assume not and that plugin authors would prefer to just have a single function that enables them access to the topic.

-- ThomasWeigert - 26 Oct 2006

Working on this code some more I come to prefer the original suggestion of improving checkTopicEditLock rather than introducing the new API. The new API, while powerful, adds a lot of work to the plugin writer that we could avoid by improving the original function. With the new API, a plugin writer would have to

  1. Copy of mimic the code in TWiki::UI::Edit::edit
  2. Copy and adapt templates/oopsleaseconflict.pattern.tmpl and friends
  3. Figure this all out (note that the use of the oops template is quite subtle; I have it not gotten to work yet due to the interference of the page redirect code in the plugin).

I will refocus on the original plan and return to the first patch on Bugs:Item2614

-- ThomasWeigert - 27 Oct 2006

I finally got this working now. There was some tedious tracing of interactions between various parts of the exception handling and redirection mechanisms in TWiki necessary. Anyway, there also were some changes needed, so please give feedback before I go any further.

  1. I decided to base the work on the original checkTopicEditLock in FuncDotPm rather than the suggestion by CC. While that suggestion could be an additional further enhancement, we need a simpler mechanism in FuncDotPm which is afforded by checkTopicEditLock.
  2. TWiki::Func::redirectCgiQuery did not support the pass through parameter, meaning that it was never possible to pass parameters across a redirect when invoking that through the Func API. This made this feature impossible, but many others also. I added that parameter. See Bugs:Item3066.
  3. There is new code in TWiki::redirect, introduced in Bugs:Item2295: When parameters are passed through, if the request_method was GET, then all parameters can be passed in the URL. If the request_method was POST then it caches the form data and passes over a cache reference in the redirect GET. This defeats the purpose of a pass through for POST methods. There may be some logic behind this, but I don't understand it. With the cache manipulation, neither the oops dialogs nor the going beyond oops dialogs works. I think this is also a bug, see Bugs:Item3067.
    • The cache mechanism for redirection has its reasons, but obviously it needs to be smarter than it is today: It just saves the CGI object before redirecting a POST request, and reads it at the target of the redirection, replacing the CGI object from the original query. This means it replaces things it shouldn't replace, e.g. $query->url(), $query->method(), and probably some other query attributes. -- HaraldJoerg - 28 Oct 2006
  4. Finally, there is a consistency issue here. As defined today, checkTopicEditLock returns a url to a target one could redirect to. A similar function, checkAccessPermission, returns a boolean and lets the client decide how to proceed once a problem is detected (throw an exception, redirect, ignore, etc.). As there are only 5 uses of checkTopicEditLock, all of them in my plugins, plus I am willing to fix the final one in EditTablePlugin, I suggest that we make these consistent and change checkTopicEditLock to return a boolean only, leaving the response up to the client. This is much more flexible, and I believe, also more proper and consistent with other TWiki functionality.

Please comment on above. I would like to wrap this up so we have this important correction to a current user problem in the code soon.

-- ThomasWeigert - 28 Oct 2006

Note that Harald inserted a comment above. We need more feedback from core developers here.

-- KennethLavrsen - 30 Oct 2006

Thomas, I proposed the get/set lease mechanism I described to avoid exactly what you propose viz changing the spec of checkTopicEditLock (your point 4). checkTopicEditLock may not be used much in published plugins, but may be in use in private plugins, and we cannot change the spec. IMHO exposing the leases through the API is significantly simpler and safer. If you want to change the function names (e.g. to setLock/getLock), fine, do that, but do not change checkTopicEditLock.

redirectCgiQuery did not support passthrough for the same reason; I didn't want to change the spec of the existing API.

The intent behind passthrough was to allow a query to be redirected to a login screen (template login) but then to restore the query context after the login was complete. For that reason I cached as much of the query as possible; we are trying to restore an original context. That is actually the recommended usage for CGI::save.

-- CrawfordCurrie - 31 Oct 2006

CC, the API for checkTopicEditLock is inconsistent and inflexible. We should change it before it gets proliferated. There are only 5 uses our there, 4 of them in my code, and one of them I already fixed in my own version of EditTablePlugin.

The get/set lease idea is flexible alright, but mabye a little too basic. It would lead to code duplication between plugin writers and core code. checkTopicEditLock seems at the right level of abstraction, and consistent with checkAccessPermitted.

redirectCgiQuery needs to be extended to support passthrough. This is just an additional parameter to the API.

Finally, your code in handling variable passing in TWiki::redirect broke the oops dialog and any other redirect that needs parameters. Also, having one-off code just supporting one situation (template login) is not wise, as for sure other uses will crop up that may be hampered by this.

-- ThomasWeigert - 31 Oct 2006

OK. I made this work without changing checkTopicEditLock but the whole thing is now waiting for a fix to the problem with caching parameters in TWiki::redirect, Bugs.Item3067

-- ThomasWeigert - 01 Nov 2006

The API for checkTopicEditLock existed years ago; before I ever got involved. So it has already been proliferated. That's why I don't want to change it.

get/set lease is not any lower level than checkTopicEditLock. The only difference is that the check function returns an oops url, and getLease returns a lease structure.

The right approach is to delete the POD doc that describes checkTopicEditLock so new uses don't emerge, and add the set/get lease functions.

-- CrawfordCurrie - 01 Nov 2006

As I said, I got the checkTopicEditLock working without changing it, but there are problems in redirect that need to be solved either way.

set/get lease is at a lower level of abstraction because it is a building block used within checkTopicEditLock, which does all the other analysis to decide whether the topic can be edited.

-- ThomasWeigert - 01 Nov 2006

After CC checked in the fixes to parameter passing, I checked in the changes to make this work. Both the Edit screen as well as plugins can now correctly recover from lease conflicts. This fixes a most annoying problem with EditTablePlugin, for example.

-- ThomasWeigert - 08 Nov 2006

Ken, you mentioned above that there are other examples of getting dumped into the Edit screen where one should not. Can you let me know where they are so that I can fix them?

-- ThomasWeigert - 08 Nov 2006

Edit | Attach | Watch | Print version | History: r28 < r27 < r26 < r25 < r24 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r28 - 2006-11-08 - ThomasWeigert
 
  • Learn about TWiki  
  • Download TWiki
This site is powered by the TWiki collaboration platform Powered by Perl Hosted by OICcam.com Ideas, requests, problems regarding TWiki? Send feedback. Ask community in the support forum.
Copyright © 1999-2017 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.