From 8e700e0ecbb088e7ede30f66c8e56a140ec11507 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 16 Oct 2023 16:57:31 -0400 Subject: [PATCH 1/8] added debugging; removed duplicate get_object calls from views --- src/registrar/models/domain.py | 30 ++++++++++++++++++++++++++++++ src/registrar/views/domain.py | 15 +++++++++------ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 59edb707a..1980bd087 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1,5 +1,6 @@ from itertools import zip_longest import logging +import inspect from datetime import date from string import digits from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -50,8 +51,33 @@ class Domain(TimeStampedModel, DomainHelper): def __init__(self, *args, **kwargs): self._cache = {} + #self.print_calling_function() + logger.info("__init__ being called on domain") super(Domain, self).__init__(*args, **kwargs) + def print_calling_function(self): + # Get the current frame in the call stack + current_frame = inspect.currentframe() + + i = 1 + while True: + try: + # Get the calling frame + calling_frame = inspect.getouterframes(current_frame, 2)[i] + + # Extract information about the calling function + calling_function_name = calling_frame.function + calling_module_name = calling_frame[0].f_globals['__name__'] + calling_line_number = calling_frame[2] + + # Print information about the calling function + print(f"Calling function: {calling_function_name} in module {calling_module_name} at line {calling_line_number}") + + i+=1 + except Exception as err: + print("========================================================") + break + class Status(models.TextChoices): """ The status codes we can receive from the registry. @@ -144,10 +170,12 @@ class Domain(TimeStampedModel, DomainHelper): def __get__(self, obj, objtype=None): """Called during get. Example: `r = domain.registrant`.""" + logger.info("domain __get__ is called: %s", obj) return super().__get__(obj, objtype) def __set__(self, obj, value): """Called during set. Example: `domain.registrant = 'abc123'`.""" + logger.info("domain __set__ is called: %s", obj) super().__set__(obj, value) # always invalidate cache after sending updates to the registry obj._invalidate_cache() @@ -1223,6 +1251,7 @@ class Domain(TimeStampedModel, DomainHelper): raise NotImplementedError() def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): + logger.info("fetch_cache called") """Contact registry for info about a domain.""" try: # get info from registry @@ -1354,6 +1383,7 @@ class Domain(TimeStampedModel, DomainHelper): def _invalidate_cache(self): """Remove cache data when updates are made.""" + logger.debug("_invalidate_cache called") self._cache = {} def _get_property(self, property): diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index d8c3c80fa..8838407f4 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -54,7 +54,7 @@ class DomainOrgNameAddressView(DomainPermissionView, FormMixin): def get_form_kwargs(self, *args, **kwargs): """Add domain_info.organization_name instance to make a bound form.""" form_kwargs = super().get_form_kwargs(*args, **kwargs) - form_kwargs["instance"] = self.get_object().domain_info + form_kwargs["instance"] = self.object.domain_info return form_kwargs def get_success_url(self): @@ -97,7 +97,7 @@ class DomainAuthorizingOfficialView(DomainPermissionView, FormMixin): def get_form_kwargs(self, *args, **kwargs): """Add domain_info.authorizing_official instance to make a bound form.""" form_kwargs = super().get_form_kwargs(*args, **kwargs) - form_kwargs["instance"] = self.get_object().domain_info.authorizing_official + form_kwargs["instance"] = self.object.domain_info.authorizing_official return form_kwargs def get_success_url(self): @@ -137,8 +137,11 @@ class DomainNameserversView(DomainPermissionView, FormMixin): def get_initial(self): """The initial value for the form (which is a formset here).""" - domain = self.get_object() + logger.info("DomainNameserversView.get_initial()") + domain = self.object + logger.info("DomainNameserversView.get_initial:: after get_object") nameservers = domain.nameservers + logger.info("DomainNameserversView.get_initial:: after set nameservers") initial_data = [] if nameservers is not None: @@ -196,7 +199,7 @@ class DomainNameserversView(DomainPermissionView, FormMixin): except KeyError: # no server information in this field, skip it pass - domain = self.get_object() + domain = self.object domain.nameservers = nameservers messages.success( @@ -257,7 +260,7 @@ class DomainSecurityEmailView(DomainPermissionView, FormMixin): def get_initial(self): """The initial value for the form.""" - domain = self.get_object() + domain = self.object initial = super().get_initial() security_contact = domain.security_contact if security_contact is None or security_contact.email == "dotgov@cisa.dhs.gov": @@ -286,7 +289,7 @@ class DomainSecurityEmailView(DomainPermissionView, FormMixin): # Set the security email from the form new_email = form.cleaned_data.get("security_email", "") - domain = self.get_object() + domain = self.object contact = domain.security_contact contact.email = new_email contact.save() From 2d4ab0c7bd24571ff4ecefe36eb8ae671b33e5bd Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 17 Oct 2023 07:57:16 -0400 Subject: [PATCH 2/8] added DomainFormBaseView and DomainBaseView and session cache for caching domains --- src/registrar/config/settings.py | 8 ++ src/registrar/models/domain.py | 4 +- src/registrar/views/domain.py | 151 ++++++++++++++++--------------- 3 files changed, 88 insertions(+), 75 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index ceb215a4d..8de4c6caa 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -169,6 +169,11 @@ WSGI_APPLICATION = "registrar.config.wsgi.application" # "BACKEND": "django.core.cache.backends.db.DatabaseCache", # } # } +CACHES = { + 'default': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + } +} # Absolute path to the directory where `collectstatic` # will place static files for deployment. @@ -652,6 +657,9 @@ SESSION_COOKIE_SAMESITE = "Lax" # instruct browser to only send cookie via HTTPS SESSION_COOKIE_SECURE = True +# session engine to cache session information +SESSION_ENGINE = 'django.contrib.sessions.backends.cache' + # ~ Set by django.middleware.clickjacking.XFrameOptionsMiddleware # prevent clickjacking by instructing the browser not to load # our site within an iframe diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index ae9d80c25..822451a49 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -61,7 +61,7 @@ class Domain(TimeStampedModel, DomainHelper): def __init__(self, *args, **kwargs): self._cache = {} - #self.print_calling_function() + self.print_calling_function() logger.info("__init__ being called on domain") super(Domain, self).__init__(*args, **kwargs) @@ -180,7 +180,7 @@ class Domain(TimeStampedModel, DomainHelper): def __get__(self, obj, objtype=None): """Called during get. Example: `r = domain.registrant`.""" - logger.info("domain __get__ is called: %s", obj) + logger.info("domain __get__ is called: %s: %s", obj, objtype) return super().__get__(obj, objtype) def __set__(self, obj, value): diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index bc1f42b88..13ca3774a 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -36,8 +36,76 @@ from .utility import DomainPermissionView, DomainInvitationPermissionDeleteView logger = logging.getLogger(__name__) +class DomainBaseView(DomainPermissionView): -class DomainView(DomainPermissionView): + def get(self, request, *args, **kwargs): + logger.info("DomainBaseView::get") + self._get_domain(request) + # pk = self.kwargs.get('pk') + # cached_domain = request.session.get(pk) + + # if cached_domain: + # logger.info("reading object from session cache") + # self.object = cached_domain + # else: + # logger.info("reading object from db") + # self.object = self.get_object() + # logger.info("writing object to session cache") + # request.session[pk] = self.object + context = self.get_context_data(object=self.object) + return self.render_to_response(context) + + def _get_domain(self, request): + # get domain from session cache or from db + # and set to self.object + # set session to self for downstream functions to + # update session cache + self.session = request.session + pk = self.kwargs.get('pk') + cached_domain = self.session.get(pk) + + if cached_domain: + logger.info("reading object from session cache") + self.object = cached_domain + else: + logger.info("reading object from db") + self.object = self.get_object() + self._update_session_with_domain() + + def _update_session_with_domain(self): + pk = self.kwargs.get('pk') + logger.info("writing object to session cache") + self.session[pk] = self.object + + +class DomainFormBaseView(DomainBaseView, FormMixin): + + def post(self, request, *args, **kwargs): + """Form submission posts to this view. + + This post method harmonizes using DetailView and FormMixin together. + """ + self._get_domain(request) + form = self.get_form() + if form.is_valid(): + return self.form_valid(form) + else: + return self.form_invalid(form) + + def form_valid(self, form): + self._update_session_with_domain() + + # superclass has the redirect + return super().form_valid(form) + + def form_invalid(self, form): + self._update_session_with_domain() + + # superclass has the redirect + return super().form_invalid(form) + + +class DomainView(DomainBaseView): """Domain detail overview page.""" @@ -46,10 +114,10 @@ class DomainView(DomainPermissionView): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - default_email = Domain().get_default_security_contact().email + default_email = self.object.get_default_security_contact().email context["default_security_email"] = default_email - security_email = self.get_object().get_security_email() + security_email = self.object.get_security_email() if security_email is None or security_email == default_email: context["security_email"] = None return context @@ -57,7 +125,7 @@ class DomainView(DomainPermissionView): return context -class DomainOrgNameAddressView(DomainPermissionView, FormMixin): +class DomainOrgNameAddressView(DomainFormBaseView): """Organization name and mailing address view""" model = Domain @@ -75,18 +143,6 @@ class DomainOrgNameAddressView(DomainPermissionView, FormMixin): """Redirect to the overview page for the domain.""" return reverse("domain-org-name-address", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): - """Form submission posts to this view. - - This post method harmonizes using DetailView and FormMixin together. - """ - self.object = self.get_object() - form = self.get_form() - if form.is_valid(): - return self.form_valid(form) - else: - return self.form_invalid(form) - def form_valid(self, form): """The form is valid, save the organization name and mailing address.""" form.save() @@ -99,7 +155,7 @@ class DomainOrgNameAddressView(DomainPermissionView, FormMixin): return super().form_valid(form) -class DomainAuthorizingOfficialView(DomainPermissionView, FormMixin): +class DomainAuthorizingOfficialView(DomainFormBaseView): """Domain authorizing official editing view.""" @@ -118,18 +174,6 @@ class DomainAuthorizingOfficialView(DomainPermissionView, FormMixin): """Redirect to the overview page for the domain.""" return reverse("domain-authorizing-official", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): - """Form submission posts to this view. - - This post method harmonizes using DetailView and FormMixin together. - """ - self.object = self.get_object() - form = self.get_form() - if form.is_valid(): - return self.form_valid(form) - else: - return self.form_invalid(form) - def form_valid(self, form): """The form is valid, save the authorizing official.""" form.save() @@ -142,7 +186,7 @@ class DomainAuthorizingOfficialView(DomainPermissionView, FormMixin): return super().form_valid(form) -class DomainNameserversView(DomainPermissionView, FormMixin): +class DomainNameserversView(DomainFormBaseView): """Domain nameserver editing view.""" @@ -191,16 +235,6 @@ class DomainNameserversView(DomainPermissionView, FormMixin): form.fields["server"].required = False return formset - def post(self, request, *args, **kwargs): - """Formset submission posts to this view.""" - self.object = self.get_object() - formset = self.get_form() - - if formset.is_valid(): - return self.form_valid(formset) - else: - return self.form_invalid(formset) - def form_valid(self, formset): """The formset is valid, perform something with it.""" @@ -224,7 +258,7 @@ class DomainNameserversView(DomainPermissionView, FormMixin): return super().form_valid(formset) -class DomainYourContactInformationView(DomainPermissionView, FormMixin): +class DomainYourContactInformationView(DomainFormBaseView): """Domain your contact information editing view.""" @@ -241,16 +275,6 @@ class DomainYourContactInformationView(DomainPermissionView, FormMixin): """Redirect to the your contact information for the domain.""" return reverse("domain-your-contact-information", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): - """Form submission posts to this view.""" - self.object = self.get_object() - form = self.get_form() - if form.is_valid(): - # there is a valid email address in the form - return self.form_valid(form) - else: - return self.form_invalid(form) - def form_valid(self, form): """The form is valid, call setter in model.""" @@ -265,7 +289,7 @@ class DomainYourContactInformationView(DomainPermissionView, FormMixin): return super().form_valid(form) -class DomainSecurityEmailView(DomainPermissionView, FormMixin): +class DomainSecurityEmailView(DomainFormBaseView): """Domain security email editing view.""" @@ -287,16 +311,6 @@ class DomainSecurityEmailView(DomainPermissionView, FormMixin): """Redirect to the security email page for the domain.""" return reverse("domain-security-email", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): - """Form submission posts to this view.""" - self.object = self.get_object() - form = self.get_form() - if form.is_valid(): - # there is a valid email address in the form - return self.form_valid(form) - else: - return self.form_invalid(form) - def form_valid(self, form): """The form is valid, call setter in model.""" @@ -327,14 +341,14 @@ class DomainSecurityEmailView(DomainPermissionView, FormMixin): return redirect(self.get_success_url()) -class DomainUsersView(DomainPermissionView): +class DomainUsersView(DomainBaseView): """User management page in the domain details.""" template_name = "domain_users.html" -class DomainAddUserView(DomainPermissionView, FormMixin): +class DomainAddUserView(DomainFormBaseView): """Inside of a domain's user management, a form for adding users. @@ -348,15 +362,6 @@ class DomainAddUserView(DomainPermissionView, FormMixin): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): - self.object = self.get_object() - form = self.get_form() - if form.is_valid(): - # there is a valid email address in the form - return self.form_valid(form) - else: - return self.form_invalid(form) - def _domain_abs_url(self): """Get an absolute URL for this domain.""" return self.request.build_absolute_uri( From 5b4103e1eed42c5662ee78018af6d911a9b71318 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 17 Oct 2023 08:43:48 -0400 Subject: [PATCH 3/8] more debugging of domain model --- src/registrar/models/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 822451a49..05cae5add 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1736,6 +1736,7 @@ class Domain(TimeStampedModel, DomainHelper): ) if property in self._cache: + logger.info("writing %s to cache", property) return self._cache[property] else: raise KeyError( From bcbf0699241c95d9634d5a8c3959b1cf1b1f4692 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 17 Oct 2023 12:24:52 -0400 Subject: [PATCH 4/8] added more logging; set hosts properly in cache when no hosts exist --- src/registrar/models/domain.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 05cae5add..8b8c8b7ce 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1450,8 +1450,10 @@ class Domain(TimeStampedModel, DomainHelper): def _fetch_hosts(self, host_data): """Fetch host info.""" + logger.info("calling _fetch_hosts on %s hosts", len(host_data)) hosts = [] for name in host_data: + logger.info("calling InfoHost on %s", name) req = commands.InfoHost(name=name) data = registry.send(req, cleaned=True).res_data[0] host = { @@ -1463,6 +1465,7 @@ class Domain(TimeStampedModel, DomainHelper): "up_date": getattr(data, "up_date", ...), } hosts.append({k: v for k, v in host.items() if v is not ...}) + logger.info("successfully called InfoHost on host_data, and have %s hosts to set to cache", len(hosts)) return hosts def _convert_ips(self, ip_list: list[str]): @@ -1593,6 +1596,8 @@ class Domain(TimeStampedModel, DomainHelper): def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): logger.info("fetch_cache called") + logger.info("fetch_hosts = %s", fetch_hosts) + logger.info("fetch_contacts = %s", fetch_contacts) """Contact registry for info about a domain.""" try: # get info from registry @@ -1629,6 +1634,7 @@ class Domain(TimeStampedModel, DomainHelper): cleaned["dnssecdata"] = extension # Capture and store old hosts and contacts from cache if they exist old_cache_hosts = self._cache.get("hosts") + logger.info("old_cache_hosts is %s", old_cache_hosts) old_cache_contacts = self._cache.get("contacts") # get contact info, if there are any @@ -1643,22 +1649,30 @@ class Domain(TimeStampedModel, DomainHelper): # hosts that existed in cache (if they existed) # and pass them along. if old_cache_hosts is not None: + logger.debug("resetting cleaned['hosts'] to old_cache_hosts") cleaned["hosts"] = old_cache_hosts # get nameserver info, if there are any if ( fetch_hosts - and "_hosts" in cleaned - and isinstance(cleaned["_hosts"], list) - and len(cleaned["_hosts"]) ): - cleaned["hosts"] = self._fetch_hosts(cleaned["_hosts"]) + if ( + "_hosts" in cleaned + and isinstance(cleaned["_hosts"], list) + and len(cleaned["_hosts"]) + ): + cleaned["hosts"] = self._fetch_hosts(cleaned["_hosts"]) + else: + cleaned["hosts"] = [] # We're only getting hosts, so retain the old # contacts that existed in cache (if they existed) # and pass them along. + logger.info("set cleaned['hosts'] to %s", cleaned["hosts"]) if old_cache_contacts is not None: + logger.info("resetting cleaned['contacts'] to old_cache_contacts") cleaned["contacts"] = old_cache_contacts # replace the prior cache with new data + logger.info("replacing the prior cache with new data") self._cache = cleaned except RegistryError as e: @@ -1729,6 +1743,7 @@ class Domain(TimeStampedModel, DomainHelper): def _get_property(self, property): """Get some piece of info about a domain.""" + logger.info("__get_property(%s)", property) if property not in self._cache: self._fetch_cache( fetch_hosts=(property == "hosts"), From e248ae7760ca27adac0a8e735e70544df8a34021 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 17 Oct 2023 21:42:43 -0400 Subject: [PATCH 5/8] remoed CACHES from settings (unnecessary); refactored _fetch_cache; added comments --- src/registrar/config/settings.py | 5 - src/registrar/models/domain.py | 154 +++++++++++-------------------- src/registrar/views/domain.py | 85 ++++++++--------- 3 files changed, 99 insertions(+), 145 deletions(-) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index d5f2b53cf..7b96af5ee 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -169,11 +169,6 @@ WSGI_APPLICATION = "registrar.config.wsgi.application" # "BACKEND": "django.core.cache.backends.db.DatabaseCache", # } # } -CACHES = { - "default": { - "BACKEND": "django.core.cache.backends.locmem.LocMemCache", - } -} # Absolute path to the directory where `collectstatic` # will place static files for deployment. diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 408721ae4..f32da2403 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1,6 +1,5 @@ from itertools import zip_longest import logging -import inspect import ipaddress import re from datetime import date @@ -62,35 +61,8 @@ class Domain(TimeStampedModel, DomainHelper): def __init__(self, *args, **kwargs): self._cache = {} - self.print_calling_function() - logger.info("__init__ being called on domain") super(Domain, self).__init__(*args, **kwargs) - def print_calling_function(self): - # Get the current frame in the call stack - current_frame = inspect.currentframe() - - i = 1 - while True: - try: - # Get the calling frame - calling_frame = inspect.getouterframes(current_frame, 2)[i] - - # Extract information about the calling function - calling_function_name = calling_frame.function - calling_module_name = calling_frame[0].f_globals["__name__"] - calling_line_number = calling_frame[2] - - # Print information about the calling function - print( - f"Calling function: {calling_function_name} in module {calling_module_name} at line {calling_line_number}" - ) - - i += 1 - except Exception as err: - print("========================================================") - break - class Status(models.TextChoices): """ The status codes we can receive from the registry. @@ -183,12 +155,10 @@ class Domain(TimeStampedModel, DomainHelper): def __get__(self, obj, objtype=None): """Called during get. Example: `r = domain.registrant`.""" - logger.info("domain __get__ is called: %s: %s", obj, objtype) return super().__get__(obj, objtype) def __set__(self, obj, value): """Called during set. Example: `domain.registrant = 'abc123'`.""" - logger.info("domain __set__ is called: %s", obj) super().__set__(obj, value) # always invalidate cache after sending updates to the registry obj._invalidate_cache() @@ -290,7 +260,6 @@ class Domain(TimeStampedModel, DomainHelper): """Creates the host object in the registry doesn't add the created host to the domain returns ErrorCode (int)""" - logger.info("Creating host") if addrs is not None: addresses = [epp.Ip(addr=addr) for addr in addrs] request = commands.CreateHost(name=host, addrs=addresses) @@ -1275,7 +1244,6 @@ class Domain(TimeStampedModel, DomainHelper): count = 0 while not exitEarly and count < 3: try: - logger.info("Getting domain info from epp") req = commands.InfoDomain(name=self.name) domainInfoResponse = registry.send(req, cleaned=True) exitEarly = True @@ -1569,10 +1537,8 @@ class Domain(TimeStampedModel, DomainHelper): def _fetch_hosts(self, host_data): """Fetch host info.""" - logger.info("calling _fetch_hosts on %s hosts", len(host_data)) hosts = [] for name in host_data: - logger.info("calling InfoHost on %s", name) req = commands.InfoHost(name=name) data = registry.send(req, cleaned=True).res_data[0] host = { @@ -1584,10 +1550,6 @@ class Domain(TimeStampedModel, DomainHelper): "up_date": getattr(data, "up_date", ...), } hosts.append({k: v for k, v in host.items() if v is not ...}) - logger.info( - "successfully called InfoHost on host_data, and have %s hosts to set to cache", - len(hosts), - ) return hosts def _convert_ips(self, ip_list: list[str]): @@ -1717,87 +1679,85 @@ class Domain(TimeStampedModel, DomainHelper): ) def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): - logger.info("fetch_cache called") - logger.info("fetch_hosts = %s", fetch_hosts) - logger.info("fetch_contacts = %s", fetch_contacts) """Contact registry for info about a domain.""" try: # get info from registry - dataResponse = self._get_or_create_domain() - data = dataResponse.res_data[0] - # extract properties from response - # (Ellipsis is used to mean "null") - cache = { - "auth_info": getattr(data, "auth_info", ...), - "_contacts": getattr(data, "contacts", ...), - "cr_date": getattr(data, "cr_date", ...), - "ex_date": getattr(data, "ex_date", ...), - "_hosts": getattr(data, "hosts", ...), - "name": getattr(data, "name", ...), - "registrant": getattr(data, "registrant", ...), - "statuses": getattr(data, "statuses", ...), - "tr_date": getattr(data, "tr_date", ...), - "up_date": getattr(data, "up_date", ...), - } - # remove null properties (to distinguish between "a value of None" and null) - cleaned = {k: v for k, v in cache.items() if v is not ...} + data_response = self._get_or_create_domain() + cache = self._extract_data_from_response(data_response) + + # remove null properties (to distinguish between "a value of None" and null) + cleaned = self._remove_null_properties(cache) - # statuses can just be a list no need to keep the epp object if "statuses" in cleaned: cleaned["statuses"] = [status.state for status in cleaned["statuses"]] - # get extensions info, if there is any - # DNSSECExtension is one possible extension, make sure to handle - # only DNSSECExtension and not other type extensions - returned_extensions = dataResponse.extensions - cleaned["dnssecdata"] = None - for extension in returned_extensions: - if isinstance(extension, extensions.DNSSECExtension): - cleaned["dnssecdata"] = extension + cleaned["dnssecdata"] = self._get_dnssec_data(data_response.extensions) + # Capture and store old hosts and contacts from cache if they exist old_cache_hosts = self._cache.get("hosts") - logger.info("old_cache_hosts is %s", old_cache_hosts) old_cache_contacts = self._cache.get("contacts") - # get contact info, if there are any - if ( - fetch_contacts - and "_contacts" in cleaned - and isinstance(cleaned["_contacts"], list) - and len(cleaned["_contacts"]) > 0 - ): - cleaned["contacts"] = self._fetch_contacts(cleaned["_contacts"]) - # We're only getting contacts, so retain the old - # hosts that existed in cache (if they existed) - # and pass them along. + if fetch_contacts: + cleaned["contacts"] = self._get_contacts( + cleaned.get("_contacts", []) + ) if old_cache_hosts is not None: logger.debug("resetting cleaned['hosts'] to old_cache_hosts") cleaned["hosts"] = old_cache_hosts - # get nameserver info, if there are any if fetch_hosts: - if ( - "_hosts" in cleaned - and isinstance(cleaned["_hosts"], list) - and len(cleaned["_hosts"]) - ): - cleaned["hosts"] = self._fetch_hosts(cleaned["_hosts"]) - else: - cleaned["hosts"] = [] - # We're only getting hosts, so retain the old - # contacts that existed in cache (if they existed) - # and pass them along. - logger.info("set cleaned['hosts'] to %s", cleaned["hosts"]) + cleaned["hosts"] = self._get_hosts( + cleaned.get("_hosts", []) + ) if old_cache_contacts is not None: - logger.info("resetting cleaned['contacts'] to old_cache_contacts") cleaned["contacts"] = old_cache_contacts - # replace the prior cache with new data - logger.info("replacing the prior cache with new data") + self._cache = cleaned except RegistryError as e: logger.error(e) + def _extract_data_from_response(self, data_response): + data = data_response.res_data[0] + cache = { + "auth_info": getattr(data, "auth_info", ...), + "_contacts": getattr(data, "contacts", ...), + "cr_date": getattr(data, "cr_date", ...), + "ex_date": getattr(data, "ex_date", ...), + "_hosts": getattr(data, "hosts", ...), + "name": getattr(data, "name", ...), + "registrant": getattr(data, "registrant", ...), + "statuses": getattr(data, "statuses", ...), + "tr_date": getattr(data, "tr_date", ...), + "up_date": getattr(data, "up_date", ...), + } + return {k: v for k, v in cache.items() if v is not ...} + + def _remove_null_properties(self, cache): + return {k: v for k, v in cache.items() if v is not ...} + + def _get_dnssec_data(self, response_extensions): + # get extensions info, if there is any + # DNSSECExtension is one possible extension, make sure to handle + # only DNSSECExtension and not other type extensions + dnssec_data = None + for extension in response_extensions: + if isinstance(extension, extensions.DNSSECExtension): + dnssec_data = extension + return dnssec_data + + def _get_contacts(self, contacts): + cleaned_contacts = {} + if contacts and isinstance(contacts, list) and len(contacts) > 0: + cleaned_contacts = self._fetch_contacts(contacts) + return cleaned_contacts + + def _get_hosts(self, hosts): + cleaned_hosts = [] + if hosts and isinstance(hosts, list): + cleaned_hosts = self._fetch_hosts(hosts) + return cleaned_hosts + def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" @@ -1863,7 +1823,6 @@ class Domain(TimeStampedModel, DomainHelper): def _get_property(self, property): """Get some piece of info about a domain.""" - logger.info("__get_property(%s)", property) if property not in self._cache: self._fetch_cache( fetch_hosts=(property == "hosts"), @@ -1871,7 +1830,6 @@ class Domain(TimeStampedModel, DomainHelper): ) if property in self._cache: - logger.info("writing %s to cache", property) return self._cache[property] else: raise KeyError( diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 5e6894bb2..fbe6a3c20 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -51,40 +51,55 @@ logger = logging.getLogger(__name__) class DomainBaseView(DomainPermissionView): + """ + Base View for the Domain. Handles getting and setting the domain + in session cache on GETs. Also provides methods for getting + and setting the domain in cache + """ + def get(self, request, *args, **kwargs): - logger.info("DomainBaseView::get") self._get_domain(request) context = self.get_context_data(object=self.object) return self.render_to_response(context) def _get_domain(self, request): - # get domain from session cache or from db - # and set to self.object - # set session to self for downstream functions to - # update session cache + """ + get domain from session cache or from db and set + to self.object + set session to self for downstream functions to + update session cache + """ self.session = request.session - pk = self.kwargs.get("pk") - cached_domain = self.session.get(pk) + # domain:private_key is the session key to use for + # caching the domain in the session + domain_pk = "domain:" + str(self.kwargs.get("pk")) + cached_domain = self.session.get(domain_pk) if cached_domain: - logger.info("reading object from session cache") self.object = cached_domain else: - logger.info("reading object from db") self.object = self.get_object() self._update_session_with_domain() def _update_session_with_domain(self): - pk = self.kwargs.get("pk") - logger.info("writing object to session cache") - self.session[pk] = self.object + """ + update domain in the session cache + """ + domain_pk = "domain:" + str(self.kwargs.get("pk")) + self.session[domain_pk] = self.object class DomainFormBaseView(DomainBaseView, FormMixin): + """ + Form Base View for the Domain. Handles getting and setting + domain in cache when dealing with domain forms. Provides + implementations of post, form_valid and form_invalid. + """ + def post(self, request, *args, **kwargs): """Form submission posts to this view. - This post method harmonizes using DetailView and FormMixin together. + This post method harmonizes using DomainBaseView and FormMixin """ self._get_domain(request) form = self.get_form() @@ -94,12 +109,14 @@ class DomainFormBaseView(DomainBaseView, FormMixin): return self.form_invalid(form) def form_valid(self, form): + # updates session cache with domain self._update_session_with_domain() # superclass has the redirect return super().form_valid(form) def form_invalid(self, form): + # updates session cache with domain self._update_session_with_domain() # superclass has the redirect @@ -200,11 +217,7 @@ class DomainNameserversView(DomainFormBaseView): def get_initial(self): """The initial value for the form (which is a formset here).""" - logger.info("DomainNameserversView.get_initial()") - domain = self.object - logger.info("DomainNameserversView.get_initial:: after get_object") - nameservers = domain.nameservers - logger.info("DomainNameserversView.get_initial:: after set nameservers") + nameservers = self.object.nameservers initial_data = [] if nameservers is not None: @@ -252,8 +265,7 @@ class DomainNameserversView(DomainFormBaseView): except KeyError: # no server information in this field, skip it pass - domain = self.object - domain.nameservers = nameservers + self.object.nameservers = nameservers messages.success( self.request, "The name servers for this domain have been updated." @@ -273,9 +285,7 @@ class DomainDNSSECView(DomainFormBaseView): """The initial value for the form (which is a formset here).""" context = super().get_context_data(**kwargs) - self.domain = self.object - - has_dnssec_records = self.domain.dnssecdata is not None + has_dnssec_records = self.object.dnssecdata is not None # Create HTML for the modal button modal_button = ( @@ -292,17 +302,16 @@ class DomainDNSSECView(DomainFormBaseView): def get_success_url(self): """Redirect to the DNSSEC page for the domain.""" - return reverse("domain-dns-dnssec", kwargs={"pk": self.domain.pk}) + return reverse("domain-dns-dnssec", kwargs={"pk": self.object.pk}) def post(self, request, *args, **kwargs): """Form submission posts to this view.""" self._get_domain(request) - self.domain = self.object form = self.get_form() if form.is_valid(): if "disable_dnssec" in request.POST: try: - self.domain.dnssecdata = {} + self.object.dnssecdata = {} except RegistryError as err: errmsg = "Error removing existing DNSSEC record(s)." logger.error(errmsg + ": " + err) @@ -326,8 +335,7 @@ class DomainDsDataView(DomainFormBaseView): def get_initial(self): """The initial value for the form (which is a formset here).""" - domain = self.object - dnssecdata: extensions.DNSSECExtension = domain.dnssecdata + dnssecdata: extensions.DNSSECExtension = self.object.dnssecdata initial_data = [] if dnssecdata is not None: @@ -368,8 +376,7 @@ class DomainDsDataView(DomainFormBaseView): # set the dnssec_ds_confirmed flag in the context for this view # based either on the existence of DS Data in the domain, # or on the flag stored in the session - domain = self.object - dnssecdata: extensions.DNSSECExtension = domain.dnssecdata + dnssecdata: extensions.DNSSECExtension = self.object.dnssecdata if dnssecdata is not None and dnssecdata.dsData is not None: self.request.session["dnssec_ds_confirmed"] = True @@ -421,9 +428,8 @@ class DomainDsDataView(DomainFormBaseView): # as valid; this can happen if form has been added but # not been interacted with; in that case, want to ignore pass - domain = self.object try: - domain.dnssecdata = dnssecdata + self.object.dnssecdata = dnssecdata except RegistryError as err: errmsg = "Error updating DNSSEC data in the registry." logger.error(errmsg) @@ -447,8 +453,7 @@ class DomainKeyDataView(DomainFormBaseView): def get_initial(self): """The initial value for the form (which is a formset here).""" - domain = self.object - dnssecdata: extensions.DNSSECExtension = domain.dnssecdata + dnssecdata: extensions.DNSSECExtension = self.object.dnssecdata initial_data = [] if dnssecdata is not None: @@ -489,8 +494,7 @@ class DomainKeyDataView(DomainFormBaseView): # set the dnssec_key_confirmed flag in the context for this view # based either on the existence of Key Data in the domain, # or on the flag stored in the session - domain = self.object - dnssecdata: extensions.DNSSECExtension = domain.dnssecdata + dnssecdata: extensions.DNSSECExtension = self.object.dnssecdata if dnssecdata is not None and dnssecdata.keyData is not None: self.request.session["dnssec_key_confirmed"] = True @@ -541,9 +545,8 @@ class DomainKeyDataView(DomainFormBaseView): except KeyError: # no server information in this field, skip it pass - domain = self.object try: - domain.dnssecdata = dnssecdata + self.object.dnssecdata = dnssecdata except RegistryError as err: errmsg = "Error updating DNSSEC data in the registry." logger.error(errmsg) @@ -596,9 +599,8 @@ class DomainSecurityEmailView(DomainFormBaseView): def get_initial(self): """The initial value for the form.""" - domain = self.object initial = super().get_initial() - security_contact = domain.security_contact + security_contact = self.object.security_contact if security_contact is None or security_contact.email == "dotgov@cisa.dhs.gov": initial["security_email"] = None return initial @@ -619,8 +621,7 @@ class DomainSecurityEmailView(DomainFormBaseView): if new_email is None or new_email.strip() == "": new_email = PublicContact.get_default_security().email - domain = self.object - contact = domain.security_contact + contact = self.object.security_contact # If no default is created for security_contact, # then we cannot connect to the registry. From a587920c4554e14c2a32a00db62d6f21419fb189 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 17 Oct 2023 21:59:33 -0400 Subject: [PATCH 6/8] bow down before the almighty linter --- src/registrar/models/domain.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index f32da2403..e444f2a61 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1698,17 +1698,13 @@ class Domain(TimeStampedModel, DomainHelper): old_cache_contacts = self._cache.get("contacts") if fetch_contacts: - cleaned["contacts"] = self._get_contacts( - cleaned.get("_contacts", []) - ) + cleaned["contacts"] = self._get_contacts(cleaned.get("_contacts", [])) if old_cache_hosts is not None: logger.debug("resetting cleaned['hosts'] to old_cache_hosts") cleaned["hosts"] = old_cache_hosts if fetch_hosts: - cleaned["hosts"] = self._get_hosts( - cleaned.get("_hosts", []) - ) + cleaned["hosts"] = self._get_hosts(cleaned.get("_hosts", [])) if old_cache_contacts is not None: cleaned["contacts"] = old_cache_contacts From 2b55e40d5793cdda88bd5991a08de2b5b10d7c5a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 18 Oct 2023 11:04:05 -0400 Subject: [PATCH 7/8] refactored _get_contacts to return default dict when no contacts exist rather then empty dict --- src/registrar/models/domain.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index e444f2a61..a212480e7 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1743,7 +1743,14 @@ class Domain(TimeStampedModel, DomainHelper): return dnssec_data def _get_contacts(self, contacts): - cleaned_contacts = {} + choices = PublicContact.ContactTypeChoices + # We expect that all these fields get populated, + # so we can create these early, rather than waiting. + cleaned_contacts = { + choices.ADMINISTRATIVE: None, + choices.SECURITY: None, + choices.TECHNICAL: None, + } if contacts and isinstance(contacts, list) and len(contacts) > 0: cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts From 94ac0e703d7cb4b8c5b42aa23c03ca9950078486 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 18 Oct 2023 13:28:50 -0400 Subject: [PATCH 8/8] small refactor to remove redundant code --- src/registrar/models/domain.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a212480e7..d0e6f1edd 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1715,7 +1715,7 @@ class Domain(TimeStampedModel, DomainHelper): def _extract_data_from_response(self, data_response): data = data_response.res_data[0] - cache = { + return { "auth_info": getattr(data, "auth_info", ...), "_contacts": getattr(data, "contacts", ...), "cr_date": getattr(data, "cr_date", ...), @@ -1727,7 +1727,6 @@ class Domain(TimeStampedModel, DomainHelper): "tr_date": getattr(data, "tr_date", ...), "up_date": getattr(data, "up_date", ...), } - return {k: v for k, v in cache.items() if v is not ...} def _remove_null_properties(self, cache): return {k: v for k, v in cache.items() if v is not ...} @@ -1821,7 +1820,6 @@ class Domain(TimeStampedModel, DomainHelper): def _invalidate_cache(self): """Remove cache data when updates are made.""" - logger.debug("_invalidate_cache called") self._cache = {} def _get_property(self, property):