Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: No redis dependency during tests and install #11880

Merged
merged 4 commits into from Dec 15, 2020
Merged

fix: No redis dependency during tests and install #11880

merged 4 commits into from Dec 15, 2020

Conversation

stephenBDT
Copy link
Contributor

Please provide enough information so that others can review your pull request:

This is just a quick little fix, so that when one adds a user in the after_install hook, the app doesn't become uninstallable anymore.

Explain the details for making this change. What existing problem does the pull request solve?

When one added a new user in the after_install hook, the app became uninstallable, as it tried to execute a background job.
This fails because redis is not available during installation of an app.

Now this is fixed by telling that background job, that it should execute immediately during tests and installs

Screenshots/GIFs

None

Adding a user during after_install hook caused error during install
of an app
@hrwX
Copy link
Contributor

hrwX commented Nov 7, 2020

@stephenBDT Do you have traceback of any sort for this ?? The update_gravatar just updates the gravatar, shouldn't make any issues for tests.

@stephenBDT
Copy link
Contributor Author

@stephenBDT Do you have traceback of any sort for this ?? The update_gravatar just updates the gravatar, shouldn't make any issues for tests.

Yes, but without the now flag, it will anyway try to execute the procedure asynchronously which does need redis and therefore fails during app installation.

@stephenBDT
Copy link
Contributor Author

stephenBDT commented Nov 9, 2020

Here is the StackTrace: @hrwX

~/frappe-bench/apps/myapp/myapp/install.py in create_initial_user()
    104     user = frappe.get_doc(user_details)
    105     user.flags.no_welcome_mail = True
--> 106     user.add_roles("System Manager")
    107 
    108 

~/frappe-bench/apps/frappe/frappe/core/doctype/user/user.py in add_roles(self, *roles)
    401                 """Add roles to user and save"""
    402                 self.append_roles(*roles)
--> 403                 self.save()
    404 
    405         def remove_roles(self, *roles):

~/frappe-bench/apps/frappe/frappe/model/document.py in save(self, *args, **kwargs)
    283         def save(self, *args, **kwargs):
    284                 """Wrapper for _save"""
--> 285                 return self._save(*args, **kwargs)
    286 
    287         def _save(self, ignore_permissions=None, ignore_version=None):

~/frappe-bench/apps/frappe/frappe/model/document.py in _save(self, ignore_permissions, ignore_version)
    305 
    306                 if self.get("__islocal") or not self.get("name"):
--> 307                         self.insert()
    308                         return
    309 

~/frappe-bench/apps/frappe/frappe/model/document.py in insert(self, ignore_permissions, ignore_links, ignore_if_duplicate, ignore_mandatory, set_name, set_child_names)
    266                 # during document creation
    267                 self.flags.update_log_for_doc_creation = True
--> 268                 self.run_post_save_methods()
    269                 self.flags.in_insert = False
    270 

~/frappe-bench/apps/frappe/frappe/model/document.py in run_post_save_methods(self)
    962 
    963                 if self._action=="save":
--> 964                         self.run_method("on_update")
    965                 elif self._action=="submit":
    966                         self.run_method("on_update")

~/frappe-bench/apps/frappe/frappe/model/document.py in run_method(self, method, *args, **kwargs)
    829 
    830                 fn.__name__ = str(method)
--> 831                 out = Document.hook(fn)(self, *args, **kwargs)
    832 
    833                 self.run_notifications(method)

~/frappe-bench/apps/frappe/frappe/model/document.py in composer(self, *args, **kwargs)
   1114 
   1115                         composed = compose(f, *hooks)
-> 1116                         return composed(self, method, *args, **kwargs)
   1117 
   1118                 return composer

~/frappe-bench/apps/frappe/frappe/model/document.py in runner(self, method, *args, **kwargs)
   1097                 def compose(fn, *hooks):
   1098                         def runner(self, method, *args, **kwargs):
-> 1099                                 add_to_return_value(self, fn(self, *args, **kwargs))
   1100                                 for f in hooks:
   1101                                         add_to_return_value(self, f(self, method, *args, **kwargs))

~/frappe-bench/apps/frappe/frappe/model/document.py in <lambda>(self, *args, **kwargs)
    823 
    824                 if hasattr(self, method) and hasattr(getattr(self, method), "__call__"):
--> 825                         fn = lambda self, *args, **kwargs: getattr(self, method)(*args, **kwargs)
    826                 else:
    827                         # hack! to run hooks even if method does not exist

~/frappe-bench/apps/frappe/frappe/core/doctype/user/user.py in on_update(self)
    108                 )
    109                 if self.name not in ('Administrator', 'Guest') and not self.user_image:
--> 110                         frappe.enqueue('frappe.core.doctype.user.user.update_gravatar', name=self.name)
    111 
    112         def has_website_permission(self, ptype, user, verbose=False):

~/frappe-bench/apps/frappe/frappe/__init__.py in enqueue(*args, **kwargs)
   1555         '''
   1556         import frappe.utils.background_jobs
-> 1557         return frappe.utils.background_jobs.enqueue(*args, **kwargs)
   1558 
   1559 def enqueue_doc(*args, **kwargs):

~/frappe-bench/apps/frappe/frappe/utils/background_jobs.py in enqueue(method, queue, timeout, event, is_async, job_name, now, enqueue_after_commit, **kwargs)
     69         else:
     70                 return q.enqueue_call(execute_job, timeout=timeout,
---> 71                         kwargs=queue_args)
     72 
     73 def enqueue_doc(doctype, name=None, method=None, queue='default', timeout=300,

~/frappe-bench/env/lib/python3.7/site-packages/rq/queue.py in enqueue_call(self, func, args, kwargs, timeout, result_ttl, ttl, failure_ttl, description, depends_on, job_id, at_front, meta, retry)
    363                         continue
    364 
--> 365         job = self.enqueue_job(job, at_front=at_front)
    366         return job
    367 

~/frappe-bench/env/lib/python3.7/site-packages/rq/queue.py in enqueue_job(self, job, pipeline, at_front)
    471         if job.timeout is None:
    472             job.timeout = self._default_timeout
--> 473         job.save(pipeline=pipe)
    474         job.cleanup(ttl=job.ttl, pipeline=pipe)
    475 

~/frappe-bench/env/lib/python3.7/site-packages/rq/job.py in save(self, pipeline, include_meta)
    583         mapping = self.to_dict(include_meta=include_meta)
    584 
--> 585         if self.get_redis_server_version() >= StrictVersion("4.0.0"):
    586             connection.hset(key, mapping=mapping)
    587         else:

~/frappe-bench/env/lib/python3.7/site-packages/rq/job.py in get_redis_server_version(self)
    591         """Return Redis server version of connection"""
    592         if not self.redis_server_version:
--> 593             self.redis_server_version = get_version(self.connection)
    594 
    595         return self.redis_server_version

~/frappe-bench/env/lib/python3.7/site-packages/rq/utils.py in get_version(connection)
    258     """
    259     try:
--> 260         version_string = connection.info("server")["redis_version"]
    261     except ResponseError:  # fakeredis doesn't implement Redis' INFO command
    262         version_string = "5.0.9"

~/frappe-bench/env/lib/python3.7/site-packages/redis/client.py in info(self, section)
   1304             return self.execute_command('INFO')
   1305         else:
-> 1306             return self.execute_command('INFO', section)
   1307 
   1308     def lastsave(self):

~/frappe-bench/env/lib/python3.7/site-packages/redis/client.py in execute_command(self, *args, **options)
    896         pool = self.connection_pool
    897         command_name = args[0]
--> 898         conn = self.connection or pool.get_connection(command_name, **options)
    899         try:
    900             conn.send_command(*args)

~/frappe-bench/env/lib/python3.7/site-packages/redis/connection.py in get_connection(self, command_name, *keys, **options)
   1190         try:
   1191             # ensure this connection is connected to Redis
-> 1192             connection.connect()
   1193             # connections that the pool provides should be ready to send
   1194             # a command. if not, the connection was either returned to the

~/frappe-bench/env/lib/python3.7/site-packages/redis/connection.py in connect(self)
    561             raise TimeoutError("Timeout connecting to server")
    562         except socket.error as e:
--> 563             raise ConnectionError(self._error_message(e))
    564 
    565         self._sock = sock

ConnectionError: Error 99 connecting to localhost:11000. Cannot assign requested address.
   1194             # a command. if not, the connection was either returned to the

~/frappe-bench/env/lib/python3.7/site-packages/redis/connection.py in connect(self)
    561             raise TimeoutError("Timeout connecting to server")
    562         except socket.error as e:
--> 563             raise ConnectionError(self._error_message(e))
    564 
    565         self._sock = sock

ConnectionError: Error 99 connecting to localhost:11000. Cannot assign requested address.

@stephenBDT
Copy link
Contributor Author

stephenBDT commented Nov 15, 2020

@hrwX please merge this, it is very easy to see, that the call to frappe.enqueue needs the now flag for the update_gravatar just as it does 3 lines above to for create_contact.

It was there for create_contact, I only added it to the second call.

Without this flag, frappe.enqueue will want to use a background worker which needs redis. This is not available during app installation

@stale
Copy link

stale bot commented Dec 12, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Dec 12, 2020
@stale stale bot removed the inactive label Dec 15, 2020
@mergify mergify bot merged commit c1665ac into frappe:develop Dec 15, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants