Page 1 of 2
Create a new exception type for unsupported operations?
Posted: 17.11.2019, 18:54
by aehimself
A couple of times I already had to determine if .Ping failed with an exception or is not supported at all. At the moment there's no other way but to check the exception class and compare the message as string.
Do you think it'd break things if we would introduce a new, EZUnsupportedException type? We would't have to worry about localization anymore, as it would be sufficient to check the exception class.
I can make the change, just wanted to ask your opinion before I do so.
Re: Create a new exception type for unsupported operations?
Posted: 17.11.2019, 20:18
by aehimself
...if we inherit it from EZSQLException, no previous code should break
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 08:32
by Fr0sT
I'd say Ping should return boolean flag and only raise exception when something really bad happens
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 09:23
by aehimself
If a method is not implemented in the driver and / or not supported by the client library, we throw an exception:
Code: Select all
function TZAbstractDbcConnection.PingServer: Integer;
begin
Result := 1;
RaiseUnsupportedException;
end;
Which is implemented like:
Code: Select all
procedure TZAbstractDbcConnection.RaiseUnsupportedException;
begin
raise EZSQLException.Create(SUnsupportedOperation);
end;
I personally agree with this way - if for example .Ping returns false it means that the server did not reply; if pinging is not supported it should be distinguished from this result.
I'm just looking for a fool-proof way to identify unsupported operations.
Edit: Being not supported is not bad enough?
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 12:16
by marsupilami
Maybe we should extend IZDatabaseInfo with a SupportsPing method?
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 13:05
by Fr0sT
marsupilami wrote:Maybe we should extend IZDatabaseInfo with a SupportsPing method?
Does it have any sense? Any server should be "pingable" via executing an empty query. And, BTW, Ping for FB and Pg doesn't re-establish the connection which is inconsistent to MySQL implementation
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 13:29
by marsupilami
Fr0sT wrote:marsupilami wrote:Maybe we should extend IZDatabaseInfo with a SupportsPing method?
Does it have any sense? Any server should be "pingable" via executing an empty query. And, BTW, Ping for FB and Pg doesn't re-establish the connection which is inconsistent to MySQL implementation
They should be pingable - yes. But as long as it is not implemented on the DBC layer, generic programs could check using the proposed IZDatabaseInfo.SupportsPing.
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 13:35
by aehimself
marsupilami wrote:Maybe we should extend IZDatabaseInfo with a SupportsPing method?
I am strongly against it.
The reason I started to think about this was the implementation of .AbortConnection, I just brought .Ping as an example. With this idea, we should create a Supports* to every method possible. This would be a huge and unnecessary work, which later might need adjusting if changes were made.
Plus, what to return for Supports*, if an earlier version of client library does not support a specific operation (thinking on your FireBird adjustment)? Yes, we could hardcode version numbers but that's something I really, really don't like.
I think raising an exception (like it is now) is the cleanest and easiest way to handle these; I just think it would be cleaner if there would be a separate exception class for not supported operations. So instead of:
Code: Select all
If (E Is EZSQLException) And (E.Message = 'Unsupported operation')
... which will not work with localizaions, we simply could verify this by
Plus, if EZUnsupportedException is defined as
Code: Select all
EZUnsupportedException = Class(EZSQLException)
no previous code should break due to (E Is EZSQLException) still should return true.
marsupilami wrote:[...] generic programs could check using the proposed IZDatabaseInfo.SupportsPing.
The way I handled this is I introduced a boolean, named "shouldping" with the initial value of True. Right next to the elapsed time between the last ping and Now I checked if we "shouldping" and that's when I called .Ping. In the exception handler if the error was unsupported, I simply toggled "shouldping" to false. Foolproof
Fr0sT wrote:Any server should be "pingable" via executing an empty query.
Deja-vu. Like if I was told it's a bad practice
http://zeoslib.sourceforge.net/viewtopi ... 99#p106099
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 14:00
by Fr0sT
aehimself wrote:Plus, what to return for Supports*, if an earlier version of client library does not support a specific operation (thinking on your FireBird adjustment)? Yes, we could hardcode version numbers but that's something I really, really don't like.
Version-dependent stuff will remain anyway... of course, we try to avoid such conditionals where possible.
Deja-vu. Like if I was told it's a bad practice
Ha-ha, and I am rather like "Whoa was that me who wrote this"
Not a bad practice until pinging happens 100 times/sec, and only for those drivers who lack other methods.
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 14:20
by aehimself
Fr0sT wrote:Version-dependent stuff will remain anyway... of course, we try to avoid such conditionals where possible.
Not necessarily. I really like Jan's correction, I'm thinking on modifying all the other implementations to use this method (except; throw an exception, not to return 1):
Supported.PNG
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 15:29
by Fr0sT
aehimself wrote:
Not necessarily. I really like Jan's correction, I'm thinking on modifying all the other implementations to use this method (except; throw an exception, not to return 1):
Sure, this method is what I meant with "avoid where possible". But there are things that couldn't be checked by availability of a function in client lib, that is, server capabilities or function parameter change.
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 16:19
by aehimself
So, what about the new exception class?
I would also like to get rid of the RaiseUnsupportedException method, as now we have like 3 versions doing the same thing, plus 10 places simply raising the exception itself
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 16:33
by Fr0sT
Personally I'm fine with new exc class as well as replacing the method, but regarding the method, Jan seems to have strong objection
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 21:39
by aehimself
Don't get me wrong: I don't have any problems with .Supports* other than the (to me, that is) seemingly unnecessary, mostly administrative load. If it is going to be implemented and will be kept up to date I'll even consider updating my prehistoric codes to use them. With that said - if I'd have to maintain a codebase as large as Zeos I would do anything to avoid this.
Maybe we should just wait for @Egonhugeist, as so far it seems to be 1 against, 1 supporting, 1 abstaining
I'll keep this idea in my stash until then.
Re: Create a new exception type for unsupported operations?
Posted: 18.11.2019, 22:27
by marsupilami
Hello,
Fr0sT wrote:but regarding the method, Jan seems to have strong objection
I have strong objections all the time - don't take that too seriously
In my opinion it is bad design, if the only way to find out if something is supported, is to try to use it and fail. The original question is:
aehimself wrote:A couple of times I already had to determine if .Ping failed with an exception or is not supported at all. At the moment there's no other way but to check the exception class and compare the message as string.
In my opinion Zeos should provide the information wether it makes sense to use the ping method at all. If you know that Ping is not supported, you don't even need to try. And if you try, you will know that an exception means a real failure to execute the Ping. You don't even need to check the exception class.
Best regards,
Jan