[Discussion] Maybe rollback should go back to the last connect/ commit method as default?

[Cheukting]

Now the WOQLClient.rollback allows a user to put in how many step to go back and 1 step is the default. Maybe the default should be going back to when the connection starts or when the user calls WOQLCLient.commit?

[matko]

Personally I'm very uncomfortable with the way we use 'commit' and 'rollback' in emulation of some other python database libraries, because our behavior is actually pretty different. Users may expect these methods to be part of a transaction mechanism and they're really not.

Regardless of whether or not you call commit, any time you do any sort of write in a query, the stuff you wrote will immediately be visible to any other clients. That is, every write is immediately a commit. So having a separate 'commit' method that is actually about some internal notion in the python client is pretty confusing. Additionally, normally 'rollback' would mean that you give up on your current transaction and decide that no other client should ever get to see the modifications you just made - but concurrent clients may have already seen all your modifications.

Furthermore, you may mean to only 'roll back' your own changes, but multiple clients may be concurrently creating new commits, so your 'roll back' (which is actually a reset of the database to some earlier commit) may actually end up undoing the work of others, which is probably not what a user may have intended to do.

I propose we don't actually override the meanings of 'commit' and 'rollback', and if we want functionality like that, it should be exposed through different methods. If we in fact want a 'commit' and 'rollback' like other clients have, we need to build support for this on the server side.

[Cheukting]

I have built it because that's what the PEP249 is calling for: https://www.python.org/dev/peps/pep-0249/

The other way to achieve it would be a change to the WOQLQuery (which will be a big one) is to separate out the query call that has the potential of making a commit (update queried) so "commit" and "query" is actually two different methods (e.g. no insert in query). The "update queries" will be stored in the client and will not send to the backend until "commit" is called, this way, rollback will only wipe out the stored "update queries"

This design is more aligned to what people are expecting, but that means it will be a rewrite and a breaking of backward compatibility

[matko]

PEP249 asks for commit and rollback to be implemented according to their normal semantics as part of transaction processing. Since that is not what we're doing, we're not actually being compliant with PEP249 at all. We're just using the same names for some methods.

From PEP249 on commit: Database modules that do not support transactions should implement this method with void functionality. Likewise, for rollback, it asks that the client throws an exception when the database does not in fact support rollback. That is not what we're doing.

Refactoring the WOQLQuery to delay all updates to the very end may work, but could also be confusing in its own way, as a user may expect data to be different after an update, but it won't be if we're holding back those updates.

I'm not opposed to breaking backwards compatibility, especially when it is in order to be more compliant with standards. That said, I think that the present 'auto-commit' behavior (so using the client without commit or rollback at all) is the most natural mapping on what actually happens in the database server, and I don't think we necessarily have to stray from that. PEP249 certainly seems to support this (just implement commit as void, and throw an exception on rollback). If we feel that more transaction logic is needed (actually needed, not just 'we wish to support more of PEP249'), we'll need some changes in the server backend to properly support it. I don't think we can hack it on the client side, because the client cannot possibly know about other clients that may concurrently be doing modifications.

[Cheukting]

I don't mind removing them, but I will rename rollback to something else cause the reset method now is a pain to use (require knowing the commit id) most people would reset by just counting how many steps back or at least there is a quick method to show all the commit history (without making a complicated WOQLQuery to the _commit graph) I took me days to write the rollback function just because I had no idea how to find the right commit id for reset.

I can go back to the drawing board for these:

1. make "commit" and "rollback" as not implemented
2. create a method to get commit history and print in a human-readable format
3. rewrite so "reset" can take either a commit or a step count to reset.

[matko]

Consider the case where there's multiple clients doing a commit. When you ask to go back 3 commits, which 3 commits do you mean? your own 3 commits, or whatever 3 commits were the latest, possibly not even your own? And if it is to undo your own commits, what do we do if some other client committed some stuff as well?
There's probably good answers to this, but we need to be clear about desired behavior here.

Also if querying the commit graph is too tricky we should figure out a way to make that easier. Maybe have a couple endpoints to do very common queries that all the client implementations are probably going to need.

[Cheukting]

That's why concurrent is tricky, for git it is straight forward cause you won't have multiple clients. if more than one client deletes the same object, what would it be? What's the solution to that at the moment? If the backend knows and will give back an error, the front end needs to have a way to let the user identify the error otherwise their application will crashes. (anyway, going too far here, what I mean is that we need to have a session to solve all the concurrent transection issues)

Seems a shorthand for getting a commit log is necessary at least. Ideally, I don't want the user to make any query to the _commit graph.

[Cheukting]

1. commit return pass
2. rollback raise a not supported error (to deprecated the old implementation)
3. A new method to return the whole commit history #183
4. reset can take in a commit id and with a flag to switch to taking in a path

[Cheukting]

1. commit return pass
2. rollback raise a not supported error (to deprecated the old implementation)
3. A new method to return the whole commit history #183
4. reset can take in a commit id and with a flag to switch to taking in a path

Item 1-3 has been dealt with. For item 4 I still have to think about the right approach.

[Cheukting]

moving no.4 to #199

1. commit return pass
2. rollback raise a not supported error (to deprecated the old implementation)
3. A new method to return the whole commit history #183
4. reset can take in a commit id and with a flag to switch to taking in a path