Singleton pattern

[Rastopapola]

Implementation

Refactors Server class to use the singleton pattern.

Benefits

• Avoid multiple instances of Server class.

Fixes

• In special cases of missing imports a NameError would normally be thrown. Somehow Django seems to rerun some internal processes where the urls.py will be rerun as well, which results in another instance of Server, which results in an AlreadyRegistered error, which is thrown and the 'real' NameError is simply dropped. This is highly confusing and will be fixed by this fix.

Tests

Tests have been run without complication. The described error scenario above can not be reproduced using the new pattern.

Please

Please check again on your machine whether tests are running fine.

[Rastopapola]

@FinalAngel P I N G Please review and merge this, I would like to update my pip configuration ;)

[FinalAngel]

I need to have a review either from @GaretJax or @kinkerl

[Rastopapola]

@FinalAngel

I need to have a review either from @GaretJax or @kinkerl

Thanks for clarification, I wasn't aware @GaretJax and @kinkerl are in charge for reviews.

[kinkerl]

Hi @Rastopapola Thanks for you contribution. Do you have more information about the Somehow Django seems to rerun some internal processes where the urls.py will be rerun as well,... - part?

[Rastopapola]

Hi @Rastopapola Thanks for you contribution. Do you have more information about the Somehow Django seems to rerun some internal processes where the urls.py will be rerun as well,... - part?

Hi @kinkerl Sadly not. I simply guessed, that some parts will be evaluated again in this scenario, since a second instance of the SsoServer would not have been created otherwise. I followed the original NameError through some layers of Django code and detected, that urls.py is evaluated a second time. This leads me to the 'conclusion' that - in case of an error thrown - Django re-runs certain parts, which involves urls.py.

Sadly I hadn't time to debug more into this 're-running' part. If you are interested, you can read my comments on the related issue and take a look for yourself. It was reproducable.

[kinkerl]

Interesting.

I don't think that the architectural change to a singleton is the right choice just to circumvent an implementation problem of the surrounding framework.

What are your issues with the try except solution? I could imagine this with an explaining comment, links to these issues and a printed warning message could be a better solution. Thoughts?

[Rastopapola]

I don't think that the architectural change to a singleton is the right choice just to circumvent an implementation problem of the surrounding framework.

I think the usage of the singleton pattern in here fits the idea of a 'server' object, which should not have multiple instances.

Since we have no idea whether the described behaviour of django is intended by the devs (maybe for a reason?) or not, it is always a good idea to harden own code against any possible drawbacks from other plugins or even the surrounding framework, as long as the own code stays compatible to existing use cases.

From my POV it's only a gain for stability to implement the server class using a pattern like singleton.

What are your issues with the try except solution? I could imagine this with an explaining comment, links to these issues and a printed warning message could be a better solution. Thoughts?

My first thought wrapped around the try-except because it seemed to be the easiest solution to overcome another registration of already registered admin classes, which caused the AlreadyRegistered error. However, imho, implementing a simple try-except with a comment about the why and when of such a situation does not prevent the situation itself. We allow the creation of a second/multiple server object/s, without really caring about the consequences. Maybe there are further drawbacks or further problems, not only the AlreadyRegistered error, that will occur due to multiple creations of the server object.

There is a larger tail of questions, which can easily be avoided by simply having only one instance of a class and each time a new instance shall be created, the same instance will be used instead.

I could imagine this with an explaining comment, links to these issues and a printed warning message could be a better solution

Of course we could add a link to the issue and this discussion in the class documentation, where the singleton pattern is implemented, so it will be clear why exactly the singleton has been added in there.

As already stated, the changes itself are rather small, if you take a look on my code. There are no real functional changes, it only affects the way the constructor reacts on a call.

[kinkerl]

Thank you for explanation. But it still does not feel like we are actually solving the root cause.

I looked at the implementation a bit and this line is interesting: https://github.com/divio/django-simple-sso/blob/d811b98afdee0e8ad011614924063ed86161ae75/simple_sso/sso_server/server.py#L142

I think our issue should be solved if we just register the admin normally in an admin.py file and not when initializing a server instance?

[Rastopapola]

I looked at the implementation a bit and this line is interesting:

Correct, this is the reason for the AlreadyRegistered error, since the register_admin() method is called right away in the constructor.

But it still does not feel like we are actually solving the root cause.

Moving the registration to somewhere else, e.g. the regular admin.py would of course solve the issue. Since this seemed odd to me, I was pretty sure you have your reasons to call the registration inside the constructor.

The singleton was my approach for keeping your functions as they are, while solving multiple calls of the constructor content.

[kinkerl]

Correct, this is the reason for the AlreadyRegistered error, since the register_admin() method is called right away in the constructor.

I feel like we should change or alter this behavior as this is already weird.

Since this seemed odd to me, I was pretty sure you have your reasons to call the registration inside the constructor.

I don't know the exact reason but in this case, the admin register depends on the instance of the server so it has to be called after the server got initiated. Maybe the initial premise to be this dynamic is unnecessary and it should not be like this this.

This being said, without reinventing the wheel or change everything drastically, we could also go for a more elegant solution without changing too much:

    def __init__(self, **kwargs):
        for key, value in kwargs.items():
            setattr(self, key, value)
        if not admin.site.is_registered(Consumer):
            self.register_admin()

[Rastopapola]

This being said, without reinventing the wheel or change everything drastically, we could also go for a more elegant solution without changing too much:

I see, you are trying to avoid the singleton at any cost ;))

Sure this solution would get rid of the double-registration problem. Since you are the devs, you need to decide on how your project should continue.

[kinkerl]

I see, you are trying to avoid the singleton at any cost ;))

Ha. :smile: I still think the "singleton" is an architectural solution but for a different kind of problem. Our goal is to register the server in the admin only once and our goal is not to get a singleton instance of the server as a design choice where the registration of the admin is just a welcomed side effect. Do you get what I mean?

In any case, I would be happy to review and merge just the if not admin.site.is_registered(Consumer) change if this resolves the problem.

[Rastopapola]

In any case, I would be happy to review and merge just the if not admin.site.is_registered(Consumer) change if this resolves the problem.

Sure that. All I'm interested is updating my virtual environment version of this package and having the issue solved ;)