Move the auth_event table out of AuthS3

[hallamoore]

Fixes #1525

This prevents the auth_event table from being loaded on every request.
The table_event getter in web2py tries looking for the table in db,
so I also had to override that getter on the AuthS3 class to look in
s3db instead.

[hallamoore]

I didn't include these when copying it over since these options aren't passed into the model function. I think that's okay?

[nursix]

This change has no actual effect performance-wise - in production mode, with lazy tables, the table will anyway only ever be instantiated if actually accessed. It's not being loaded on every request, no.

So, it's optimizing a developer setup - but in the most developer-unfriendly way, by deviating from the super-class at the expense of introducing a workaround to make it even possible. IMHO this turns a good intention into a bad idea.

I would therefore say no to the entire motion here, i.e. drop it.

[flavour]

I would prefer to keep it personally...the performance advantage outweighs the extra subclass over-ride.
@nursix : Do you really want to veto this?

[nursix]

Yes, I would "veto" it - if there were anything like a veto ;)

Like I said: there is no measurable performance advantage - not in the developer setup, and even less in production mode for aforementioned reasons. The hack to make it work with web2py internals is confusing, and we shouldn't really mess with that - otherwise it could happen that we need compat-hacks for different web2py versions in the future...complications over complications.

But then again, there is no "veto" here - just a strong opinion based on long-term experience with web2py and Eden optimization. In my eyes, this goes overboard here for nothing.

If you have real evidence that there is a significant performance advantage, I might change my mind - but I for one couldn't find any.

[nursix]

@hallamoore : As you can see, #1525 isn't undisputed. I should have posted my concerns there already, but missed the opportunity as I got distracted by other things.

My main concern here is that we move the definition of a web2py framework entity entirely out of the subclass. Web2py have made structural changes to this API in the past, and when they would do so again in the future, we may be forced to introduce compatibility workarounds in order to support multiple versions (or otherwise force deployments to upgrade web2py, which isn't nice). I've had to do such compat workarounds in the past, and they are messy, and not exactly helpful with performance.

However, I agree that running the table definition for every request is unnecessary - although I still doubt the relevance of the performance impact it has, it's more that I agree based on the principle of laziness, i.e. don't do things unless you must.

If you have a suggestion how to resolve the dispute, that would be very welcome. If not, then maybe you have an opinion or arguments towards either side? Compelling evidence that the performance gain is worth the risk of future complications?

The minimum change that I would request here is this, though:

1. Do not remove the table definition in AuthS3.define_tables, but comment it out, with an explanation where it has been moved and why
2. Add a docstring to the AuthS3.table_event getter that explains what it is for and why it has been overridden

This helps us to remember if we need to adapt to future web2py changes, which could be years away, and we're senile and forgetful ;) so need to have such traces in the code.

[flavour]

My main concern here is that we move the definition of a web2py framework entity entirely out of the subclass.

The problem with anything like just defining the table in the getter is that then we need to handle table migrations separately...having this in s3db means everything just works smoothly

Web2py have made structural changes to this API in the past, and when they would do so again in the future, we may be forced to introduce compatibility workarounds in order to support multiple versions

My sense is that we're already at a point where upgrading to the latest Auth is non-trivial (& I'd like to do it at some point for 2-factor support)...I see this as very unlikely to be a big straw causing significant issues.

If you have a suggestion how to resolve the dispute, that would be very welcome. If not, then maybe you have an opinion or arguments towards either side?

Yup, thoughts/suggestions welcomed :)

[hallamoore]

The override does feel a little developer unfriendly to me, but I've found web2py to be developer unfriendly as a whole so far. 😞 Extremely hard to follow unless you just know-how-it-works™️

The fact that this doesn't have any effect on prod makes me lean towards dropping it, but I don't have much perspective on how much developer pain is caused by this.

[nursix]

Given that you have both alternatives available, I think you can test how much of a "pain" is caused by having this model in AuthS3.define_tables vs. s3db: if you compare the average response times in each variant of a developer instance with debug/migrate on.

If that gives a significant advantage for the s3db-variant, then you have the necessary evidence that this change indeed improves something.

[nursix]

And I would really prefer to have this modification justified/backed by some evidence.