public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashesh Vashi <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: Refactor: Registry Classes
Date: Wed, 23 Jun 2021 19:54:32 +0530
Message-ID: <CAG7mmow6uPDAhfELmyXjBziHti0dE6aYCM7ufJZS3VXYZoLT-A@mail.gmail.com> (raw)
In-Reply-To: <CANxoLDcUuG1+yO0nGjXvZGwxnpofwyfsBNjOimSbvr40-6UPFw@mail.gmail.com>
References: <CAG7mmoz=wOOc3QiZDZuSwonFue_sowcZRtZLK_M0o839ZPnJ_Q@mail.gmail.com>
<CAG7mmoxQfhVhGUC2ArMkCPUoWiBusu-z6=0PR8OiZfOMQpnWwg@mail.gmail.com>
<CANxoLDcUuG1+yO0nGjXvZGwxnpofwyfsBNjOimSbvr40-6UPFw@mail.gmail.com>
On Wed, Jun 23, 2021 at 1:22 PM Akshay Joshi <[email protected]>
wrote:
> Hi Ashesh
>
> Following are the review comments:
>
> - Fixed PEP8 issues.
>
> Done.
>
> - In "dynamic_registry/__init__.py" decorator @classmethod used for "
> *_get*" and "*_load_modules*" methods which are actually outside of
> the class. Even constructor also outside of the class.
>
> 'create_registry_metaclass' is not a class, but a method to create the
dynamic classes.
If I move these methods in 'create_registry_metaclass' method, SonarQube
raises issues about complexity of the functions, hence - they're best kept
outside of that method.
>
> - Remove unused imports from "driver/registry.py"
>
> Done
>
> -
> - Fixed sonarqube issues in
> "dynamic_registry/tests/registry/__init__.py"
>
> Done
As discussed, SonarQube is not able to understand that the result object is
a class, and not an object, hence - showing linter issues.
I've disabled them in those lines by adding the comment '# NOSNAR' at the
end.
-- Thanks, Ashesh
>
> On Sat, Jun 19, 2021 at 11:27 AM Ashesh Vashi <
> [email protected]> wrote:
>
>> On Sat, Jun 19, 2021 at 1:39 AM Ashesh Vashi <
>> [email protected]> wrote:
>>
>>> Hi Akshay,
>>>
>>> We do have a couple of classes, which does automatic registration of the
>>> base classes,
>>> and which creates single-ton objects for these base classes, when needed.
>>>
>>> I would be working on a patch sooner, which will be using similar
>>> functionality for loading
>>> the multi-factor authentication.
>>>
>>> I realized - it will be a duplicate code at three places for the same
>>> functionalities.
>>> Hence - I worked on refactoring this registry class.
>>>
>>> Please find the patch for the same.
>>>
>> Found issues - some test files were using the old function
>> 'Driver.load_drivers(...)'.
>> They're fixed now.
>>
>> -- Thanks, Ashesh
>>
>>>
>>> --
>>>
>>> Thanks & Regards,
>>>
>>> Ashesh Vashi
>>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>>> <http://www.enterprisedb.com;
>>>
>>>
>>> *http://www.linkedin.com/in/asheshvashi*
>>> <http://www.linkedin.com/in/asheshvashi;
>>>
>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Principal Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>
Attachments:
[application/octet-stream] refactor_registry_class_v3.patch (19.7K, 3-refactor_registry_class_v3.patch)
download | inline diff:
diff --git a/web/pgadmin/authenticate/__init__.py b/web/pgadmin/authenticate/__init__.py
index d328b0d24..101d57656 100644
--- a/web/pgadmin/authenticate/__init__.py
+++ b/web/pgadmin/authenticate/__init__.py
@@ -223,7 +223,7 @@ def get_auth_sources(type):
if type in auth_sources:
return auth_sources[type]
- auth_source = AuthSourceRegistry.create(type)
+ auth_source = AuthSourceRegistry.get(type)
if auth_source is not None:
auth_sources[type] = auth_source
@@ -236,7 +236,7 @@ def init_app(app):
auth_sources = dict()
setattr(app, '_pgadmin_auth_sources', auth_sources)
- AuthSourceRegistry.load_auth_sources()
+ AuthSourceRegistry.load_modules(app)
return auth_sources
diff --git a/web/pgadmin/authenticate/registry.py b/web/pgadmin/authenticate/registry.py
index 7d797e658..84c5fd07a 100644
--- a/web/pgadmin/authenticate/registry.py
+++ b/web/pgadmin/authenticate/registry.py
@@ -10,56 +10,9 @@
"""External Authentication Registry."""
-from flask_babelex import gettext
-from abc import ABCMeta
+from pgadmin.utils.dynamic_registry import create_registry_metaclass
-def _decorate_cls_name(module_name):
- length = len(__package__) + 1
-
- if len(module_name) > length and module_name.startswith(__package__):
- return module_name[length:]
-
- return module_name
-
-
-class AuthSourceRegistry(ABCMeta):
- registry = None
- auth_sources = dict()
-
- def __init__(self, name, bases, d):
-
- # Register this type of auth_sources, based on the module name
- # Avoid registering the BaseAuthentication itself
-
- AuthSourceRegistry.registry[_decorate_cls_name(d['__module__'])] = self
- ABCMeta.__init__(self, name, bases, d)
-
- @classmethod
- def create(cls, name, **kwargs):
-
- if name in AuthSourceRegistry.auth_sources:
- return AuthSourceRegistry.auth_sources[name]
-
- if name in AuthSourceRegistry.registry:
- AuthSourceRegistry.auth_sources[name] = \
- (AuthSourceRegistry.registry[name])(**kwargs)
- return AuthSourceRegistry.auth_sources[name]
-
- raise NotImplementedError(
- gettext(
- "Authentication source '{0}' has not been implemented."
- ).format(name)
- )
-
- @classmethod
- def load_auth_sources(cls):
- # Initialize the registry only if it has not yet been initialized
- if AuthSourceRegistry.registry is None:
- AuthSourceRegistry.registry = dict()
-
- from importlib import import_module
- from werkzeug.utils import find_modules
-
- for module_name in find_modules(__package__, True):
- import_module(module_name)
+AuthSourceRegistry = create_registry_metaclass(
+ "AuthSourceRegistry", __package__, decorate_as_module=True
+)
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/foreign_key/sql/tests/test_foreign_key_properties.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/foreign_key/sql/tests/test_foreign_key_properties.py
index 7c9d8f423..4a6c2664b 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/foreign_key/sql/tests/test_foreign_key_properties.py
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/foreign_key/sql/tests/test_foreign_key_properties.py
@@ -14,7 +14,7 @@ from regression.python_test_utils.template_helper import file_as_template
from pgadmin.utils.route import BaseTestGenerator
from regression.python_test_utils import test_utils
-DriverRegistry.load_drivers()
+DriverRegistry.load_modules()
class TestColumnForeignKeyGetConstraintCols(BaseTestGenerator):
diff --git a/web/pgadmin/utils/driver/__init__.py b/web/pgadmin/utils/driver/__init__.py
index 5c6d6945a..d7748c839 100644
--- a/web/pgadmin/utils/driver/__init__.py
+++ b/web/pgadmin/utils/driver/__init__.py
@@ -12,32 +12,19 @@ from flask import current_app
from .registry import DriverRegistry
-def get_driver(type, app=None):
- if app is not None:
- DriverRegistry.load_drivers()
-
- drivers = getattr(app or current_app, '_pgadmin_server_drivers', None)
-
- if drivers is None or not isinstance(drivers, dict):
- drivers = dict()
+def get_driver(_type, app=None):
- if type in drivers:
- return drivers[type]
-
- driver = DriverRegistry.create(type)
-
- if driver is not None:
- drivers[type] = driver
- setattr(app or current_app, '_pgadmin_server_drivers', drivers)
+ if app is not None:
+ DriverRegistry.load_modules(app)
- return driver
+ return DriverRegistry.get(_type)
def init_app(app):
drivers = dict()
setattr(app, '_pgadmin_server_drivers', drivers)
- DriverRegistry.load_drivers()
+ DriverRegistry.load_modules(app)
return drivers
diff --git a/web/pgadmin/utils/driver/registry.py b/web/pgadmin/utils/driver/registry.py
index ae535429b..7ee5ee89b 100644
--- a/web/pgadmin/utils/driver/registry.py
+++ b/web/pgadmin/utils/driver/registry.py
@@ -9,80 +9,9 @@
from abc import ABCMeta
-from flask_babelex import gettext
+from pgadmin.utils.dynamic_registry import create_registry_metaclass
-def _decorate_cls_name(module_name):
- length = len(__package__) + 1
-
- if len(module_name) > length and module_name.startswith(__package__):
- return module_name[length:]
-
- return module_name
-
-
-class DriverRegistry(ABCMeta):
- """
- class DriverRegistry(object)
- Every Driver will be registered automatically by its module name.
-
- This uses factory pattern to genreate driver object based on its name
- automatically.
-
- Class-level Methods:
- ----------- -------
- * __init__(...)
- - It will be used to register type of drivers. You don't need to call
- this function explicitly. This will be automatically executed, whenever
- we create class and inherit from BaseDriver, it will register it as
- available driver in DriverRegistry. Because - the __metaclass__ for
- BaseDriver is set it to DriverRegistry, and it will create new instance
- of this DriverRegistry per class.
-
- * create(type, *args, **kwargs)
- - Create type of driver object for this server, from the available
- driver list (if available, or raise exception).
-
- * load_drivers():
- - Use this function from init_app(...) to load all available drivers in
- the registry.
- """
- registry = None
- drivers = dict()
-
- def __init__(self, name, bases, d):
-
- # Register this type of driver, based on the module name
- # Avoid registering the BaseDriver itself
-
- if name != 'BaseDriver':
- DriverRegistry.registry[_decorate_cls_name(d['__module__'])] = self
-
- ABCMeta.__init__(self, name, bases, d)
-
- @classmethod
- def create(cls, name, **kwargs):
-
- if name in DriverRegistry.drivers:
- return DriverRegistry.drivers[name]
-
- if name in DriverRegistry.registry:
- DriverRegistry.drivers[name] = \
- (DriverRegistry.registry[name])(**kwargs)
- return DriverRegistry.drivers[name]
-
- raise NotImplementedError(
- gettext("Driver '{0}' has not been implemented.").format(name)
- )
-
- @classmethod
- def load_drivers(cls):
- # Initialize the registry only if it has not yet been initialized
- if DriverRegistry.registry is None:
- DriverRegistry.registry = dict()
-
- from importlib import import_module
- from werkzeug.utils import find_modules
-
- for module_name in find_modules(__package__, True):
- import_module(module_name)
+DriverRegistry = create_registry_metaclass(
+ "DriverRegistry", __package__, decorate_as_module=True
+)
diff --git a/web/pgadmin/utils/dynamic_registry/__init__.py b/web/pgadmin/utils/dynamic_registry/__init__.py
new file mode 100644
index 000000000..dabde3a95
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/__init__.py
@@ -0,0 +1,104 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+#########################################################################
+
+"""This file contains functions for creating dynamic registry meta class."""
+
+from abc import ABCMeta
+
+
+# Constructor
+def __constructor(self, name, bases, kwargs):
+ # Register this type of auth_sources, based on the module name
+ # Avoid registering the BaseAuthentication itself
+ cls = self.__class__
+ entry = self._decorate_cls_name(name, kwargs)
+
+ if cls._registry is None:
+ cls._registry = dict()
+ else:
+ if entry in cls._registry:
+ raise RuntimeError((
+ "{} class is already been registered with {} "
+ "(package: {})"
+ ).format(entry, cls._name_, cls._package_))
+
+ if cls._initialized is True:
+ cls._registry[entry] = self
+ cls._initialized = True
+
+ ABCMeta.__init__(self, name, bases, kwargs)
+
+
+@classmethod
+def __get(cls, name, **kwargs):
+ if name in cls._objects:
+ return cls._objects[name]
+
+ if cls._registry is not None and name in cls._registry:
+ cls._objects[name] = (cls._registry[name])(**kwargs)
+
+ return cls._objects[name]
+
+ raise NotImplementedError(
+ "{} '{}' has not been implemented.".format(cls.__name__, name)
+ )
+
+
+@classmethod
+def __load_modules(cls, app=None):
+ # Initialize the registry only if it has not yet been initialized
+ if cls._registry is None:
+ cls._registry = dict()
+
+ from importlib import import_module
+ from werkzeug.utils import find_modules
+
+ for module_name in find_modules(cls._package_, True):
+ module = import_module(module_name)
+
+ if "init_app" in module.__dict__.keys():
+ module.__dict__["init_app"](app)
+
+
+def __get_module_name(self, name, kwargs):
+ module_name = kwargs["__module__"]
+ length = len(self._package_) + 1
+
+ if len(module_name) > length and module_name.startswith(self._package_):
+ return module_name[length:]
+
+ return module_name
+
+
+def __get_class_name(self, name, kwargs):
+ return name
+
+
+def create_registry_metaclass(name, package, decorate_as_module=True):
+
+ class_params = {
+ # constructor
+ "__init__": __constructor,
+
+ # Class members
+ "_registry": None,
+ "_objects": dict(),
+ "_package_": package,
+
+ # Member functions
+ "get": __get,
+ "load_modules": __load_modules,
+ "_name_": name,
+ "_decorate_cls_name": __get_module_name
+ if decorate_as_module is True else __get_class_name,
+ "_initialized": False,
+ }
+
+ # Creating class dynamically
+ return type(package + "." + name, (ABCMeta, ), class_params)
diff --git a/web/pgadmin/utils/dynamic_registry/tests/__init__.py b/web/pgadmin/utils/dynamic_registry/tests/__init__.py
new file mode 100644
index 000000000..89190b316
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/__init__.py
@@ -0,0 +1,8 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
diff --git a/web/pgadmin/utils/dynamic_registry/tests/registry/__init__.py b/web/pgadmin/utils/dynamic_registry/tests/registry/__init__.py
new file mode 100644
index 000000000..d6455b67f
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/registry/__init__.py
@@ -0,0 +1,99 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+import six
+from pgadmin.utils.dynamic_registry import create_registry_metaclass
+from .registry import TestModuleRegistry, TestNamedRegistry, TestNameBase
+from .test1 import TestModule1
+
+
+def test_load_modules_based_registry():
+ TestModuleRegistry.load_modules()
+
+ if len(TestModuleRegistry._registry) != 2:
+ return "Length of the registry should have been 2"
+
+ if 'test1' not in TestModuleRegistry._registry:
+ return "'test1' is not found in the registry"
+
+ if 'test2' not in TestModuleRegistry._registry:
+ return "'test2' is not found in the registry"
+
+ obj_test1_1 = TestModuleRegistry.get('test1')
+ obj_test1_2 = TestModuleRegistry.get('test1')
+ obj_test2_1 = TestModuleRegistry.get('test2')
+
+ if id(obj_test1_1) != id(obj_test1_2):
+ return "Registry has created two separate instances"
+
+ if isinstance(obj_test1_1, TestModule1) is False:
+ return "Registry created wrong object"
+
+ if id(obj_test1_1) == id(obj_test2_1):
+ return "Registry should have created a separate instances for " \
+ "different classes"
+
+
+def test_load_classname_registry():
+ TestNamedRegistry.load_modules()
+
+ if 'ClassTestName1' not in TestNamedRegistry._registry:
+ return "'ClassTestName1' is not found in the registry"
+
+ if 'ClassTestName2' not in TestNamedRegistry._registry:
+ return "'ClassTestName2' is not found in the registry"
+
+ obj_test1_1 = TestNamedRegistry.get('ClassTestName1')
+ obj_test1_2 = TestNamedRegistry.get('ClassTestName1')
+ obj_test2_1 = TestNamedRegistry.get('ClassTestName2')
+
+ if id(obj_test1_1) != id(obj_test1_2):
+ return "Registry has created two separate instances"
+
+ if id(obj_test1_1) == id(obj_test2_1):
+ return "Registry should have created a separate instances for " \
+ "different classes"
+
+ try:
+ class ClassTestName1(TestNameBase): # NOSONAR
+ pass
+
+ return "Expected an runtime error when using the same classname"
+ except RuntimeError:
+ pass
+
+
+def test_empty_registry():
+
+ EmptyModuleRegistry = create_registry_metaclass( # NOSONAR
+ "EmptyModuleRegistry", __package__, decorate_as_module=True
+ )
+
+ if EmptyModuleRegistry._registry is not None:
+ return "Registry was supposed to be None"
+
+ if EmptyModuleRegistry._initialized is not False:
+ return "Registry initialized flag should be false"
+
+
+def test_create_base_class():
+ RegistryWithBaseClass = create_registry_metaclass( # NOSONAR
+ 'RegistryWithBaseClass', __package__, decorate_as_module=False
+ )
+
+ @six.add_metaclass(RegistryWithBaseClass)
+ class TestBase(object):
+ pass
+
+ registry = RegistryWithBaseClass._registry
+
+ if registry is None or len(registry) != 0:
+ return "Registry was not supposed to be None, and empty"
+
+ if RegistryWithBaseClass._initialized is False:
+ return "Registry initialized flag should not be true"
diff --git a/web/pgadmin/utils/dynamic_registry/tests/registry/registry.py b/web/pgadmin/utils/dynamic_registry/tests/registry/registry.py
new file mode 100644
index 000000000..ac83beebc
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/registry/registry.py
@@ -0,0 +1,38 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+import six
+from pgadmin.utils.dynamic_registry import create_registry_metaclass
+
+
+TestModuleRegistry = create_registry_metaclass(
+ 'TestModuleRegistry', __package__, decorate_as_module=True
+)
+
+
+TestNamedRegistry = create_registry_metaclass(
+ 'TestRegistry', __package__, decorate_as_module=False
+)
+
+
[email protected]_metaclass(TestModuleRegistry)
+class TestModuleBase(object):
+ pass
+
+
[email protected]_metaclass(TestNamedRegistry)
+class TestNameBase(object):
+ pass
+
+
+class ClassTestName1(TestNameBase):
+ pass
+
+
+class ClassTestName2(TestNameBase):
+ pass
diff --git a/web/pgadmin/utils/dynamic_registry/tests/registry/test1.py b/web/pgadmin/utils/dynamic_registry/tests/registry/test1.py
new file mode 100644
index 000000000..8f8f25db0
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/registry/test1.py
@@ -0,0 +1,15 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+from .registry import TestModuleBase
+
+
+# This class will be registered with TestModuleRegistry (registry)
+class TestModule1(TestModuleBase):
+ pass
diff --git a/web/pgadmin/utils/dynamic_registry/tests/registry/test2.py b/web/pgadmin/utils/dynamic_registry/tests/registry/test2.py
new file mode 100644
index 000000000..c9b045895
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/registry/test2.py
@@ -0,0 +1,14 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+from .registry import TestModuleBase
+
+
+# This class will be registered with TestModuleRegistry (registry)
+class TestModule2(TestModuleBase):
+ pass
diff --git a/web/pgadmin/utils/dynamic_registry/tests/test_dynamic_registry.py b/web/pgadmin/utils/dynamic_registry/tests/test_dynamic_registry.py
new file mode 100644
index 000000000..52136b069
--- /dev/null
+++ b/web/pgadmin/utils/dynamic_registry/tests/test_dynamic_registry.py
@@ -0,0 +1,39 @@
+#######################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2021, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+from pgadmin.utils.route import BaseTestGenerator
+from .registry import test_load_modules_based_registry, test_empty_registry, \
+ test_create_base_class, test_load_classname_registry
+
+
+class TestDynamicRegistry(BaseTestGenerator):
+
+ scenarios = [
+ (
+ "Check empty registry",
+ dict(test=test_empty_registry),
+ ),
+ (
+ 'Load the registry based on the modules',
+ dict(test=test_load_modules_based_registry),
+ ),
+ (
+ 'Load the registry based on the name of the classes',
+ dict(test=test_load_classname_registry),
+ ),
+ (
+ "When created a base class registry is initialized",
+ dict(test=test_create_base_class),
+ ),
+ ]
+
+ def runTest(self):
+ error = self.test()
+
+ if error is not None:
+ self.fail(error)
diff --git a/web/regression/python_test_utils/sql_template_test_base.py b/web/regression/python_test_utils/sql_template_test_base.py
index e3a381675..2b9fe76b0 100644
--- a/web/regression/python_test_utils/sql_template_test_base.py
+++ b/web/regression/python_test_utils/sql_template_test_base.py
@@ -15,7 +15,7 @@ from pgadmin.utils.driver import DriverRegistry
from pgadmin.utils.versioned_template_loader \
import get_version_mapping_directories
-DriverRegistry.load_drivers()
+DriverRegistry.load_modules()
class SQLTemplateTestBase(BaseTestGenerator):
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Refactor: Registry Classes
In-Reply-To: <CAG7mmow6uPDAhfELmyXjBziHti0dE6aYCM7ufJZS3VXYZoLT-A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox