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

DPL-1100 - As developers we need to fix and merge the dependencies to reduce security problems and reduce techncial debt #909

Closed
stevieing opened this issue Feb 16, 2024 · 5 comments · Fixed by #890
Assignees
Labels
dependencies Pull requests that update a dependency file Size: M Medium - medium effort & risk Value: 3 Value to the insitute is average

Comments

@stevieing
Copy link
Contributor

Describe the Housekeeping
As developers we need to fix and merge the dependencies to reduce security problems and reduce techncial debt

Blocking issues
Describe any other issues or tickets that may be blocking this change.

Additional context

@stevieing stevieing added dependencies Pull requests that update a dependency file Size: M Medium - medium effort & risk Value: 3 Value to the insitute is average labels Feb 16, 2024
@dasunpubudumal
Copy link
Contributor

dasunpubudumal commented Feb 20, 2024

Attaching some finds on #890:

The error message from CI log is attached below:

=================================== FAILURES ===================================
___________________ test_run_mysql_executemany_query_success ___________________
config = <module 'crawler.config.test' from '/home/runner/work/crawler/crawler/crawler/config/test.py'>
    def test_run_mysql_executemany_query_success(config):
>       conn = MockMySQLConnection()
E       TypeError: Can't instantiate abstract class MockMySQLConnection with abstract methods cmd_change_user, cmd_debug, cmd_init_db, cmd_ping, cmd_process_kill, cmd_query, cmd_query_iter, cmd_quit, cmd_refresh, cmd_reset_connection, cmd_shutdown, cmd_statistics, cmd_stmt_close, cmd_stmt_execute, cmd_stmt_prepare, cmd_stmt_reset, cmd_stmt_send_long_data, connection_id, get_row
tests/db/test_mysql.py:55: TypeError

The update from 8.2.0 to 8.3.0 adds an additional decorator @abstractmethod (coming from abc module (i.e., Abstract Base Classes)) for the abstract methods in the superclass MySQLConnectionAbstract, that we use to create a MockMySQLConnection via inheritance. I didn't see this change in their changelog, but was able to find out by comparing the source code versions.

image

If a subclass that does not implement all the methods decorated with @abstractmethod is instantiated, Python interpreter will throw out a TypeError, as it was designed to do so.

I do not think that this will affect our database connection initiation logic in our business logic, as it uses the helper function mysql.connect() that returns a MySQLConnection object (which implements the abstract methods), but it will require code changes in tests when we're trying to initiate a mock connection.


Tests were fixed via the following code changes:

  • Adding the following mocks in tests/db/test_mysql.py
  cmd_change_user = MagicMock()
  cmd_debug = MagicMock()
  cmd_init_db = MagicMock()
  cmd_ping = MagicMock()
  cmd_process_kill = MagicMock()
  cmd_query = MagicMock()
  cmd_query_iter = MagicMock()
  cmd_quit = MagicMock()
  cmd_refresh = MagicMock()
  cmd_reset_connection = MagicMock()
  cmd_shutdown = MagicMock()
  cmd_statistics = MagicMock()
  cmd_stmt_close = MagicMock()
  cmd_stmt_execute = MagicMock()
  cmd_stmt_prepare = MagicMock()
  cmd_stmt_reset = MagicMock()
  cmd_stmt_send_long_data = MagicMock()
  connection_id = MagicMock()
  get_row = MagicMock()

@dasunpubudumal
Copy link
Contributor

dasunpubudumal commented Feb 20, 2024

Furthermore, the upgrade from 8.2.0 to 8.3.0 carries significant type changes that heavily affects mypy. Two (out of many) such examples are shown below.

Screenshot 2024-02-20 at 5 25 34 pm image

We might have to update crawler function signatures and function calls to adhere to the upgrades; otherwise, mypy will complain regarding typing information.

@dasunpubudumal
Copy link
Contributor

The changes discussed in #909 (comment) and #909 (comment) were completed and merged into develop (and master) with v2.13.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Size: M Medium - medium effort & risk Value: 3 Value to the insitute is average
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants