Fixes lane saving bug

[dbernstein]

Description

This PR resolves the bug that was manifesting as a failure to save lanes.

I'm not entirely why not passing the search_engine argument without a key causes the following error:

{"host": "eb21d6c68737", "name": "core.app_server.ErrorHandler", "level": "ERROR", "filename": "app_server.py", "message": "Exception in web app: Lane.update_size() got multiple values for argument 'search_engine'", "timestamp": "2024-02-20T23:43:55.588382+00:00", "traceback": "Traceback (most recent call last):\n  File \"/var/www/circulation/env/lib/python3.10/site-packages/flask/app.py\", line 870, in full_dispatch_request\n    rv = self.dispatch_request()\n  File \"/var/www/circulation/env/lib/python3.10/site-packages/flask/app.py\", line 855, in dispatch_request\n    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]\n  File \"/var/www/circulation/api/routes.py\", line 115, in decorated\n    return f(*args, **kwargs)\n  File \"/var/www/circulation/api/admin/routes.py\", line 93, in decorated\n    v = f(*args, **kwargs)\n  File \"/var/www/circulation/api/admin/routes.py\", line 64, in decorated\n    return f(*args, **kwargs)\n  File \"/var/www/circulation/api/admin/routes.py\", line 82, in decorated\n    return f(*args, **kwargs)\n  File \"/var/www/circulation/api/admin/routes.py\", line 658, in lanes\n    return app.manager.admin_lanes_controller.lanes()\n  File \"/var/www/circulation/api/admin/controller/lanes.py\", line 163, in lanes\n    lane.update_size(self._db, self.search_engine)\n  File \"src/dependency_injector/_cwiring.pyx\", line 28, in dependency_injector._cwiring._get_sync_patched._patched\nTypeError: Lane.update_size() got multiple values for argument 'search_engine'"}

When I debugged it, there was only one value being passed. I'm guessing it has something to do with the @Inject tag trying to inject the available search_engine in addition to the explicit self.search_engine.

I noticed that removing the self.seach_engine argument altogether also solved the problem which makes sense because it is injecting the available search_engine in the context. Presumably the injected search_engine instance is the same instance being passed in explicitly. The unit tests pass because the search_engine is explicitly using the keyword. I'm not seeing it being passed implicitly via injection anywhere else in the code.

For the sake of completeness, I also went ahead and updated other references to Lane.update_size where the search_engine wasn't being passed with the keyword.

@jonathangreen : do you have any insight into what we should be doing here? Should rely on the injection or is it better to be explicit?

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-986

How Has This Been Tested?

Manually tested on a local instance. Unit tests still pass.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[jonathangreen]

@dbernstein the reason its happening, as you suspected, is the @inject decorator. It always injects args using the keyword form. So the function is being called with both a positional and keyword arg for the search_engine parameter.

I think generally explicit is better then implicit for arguments, so I'd be happy to drop the @inject decorator here. I've been trying to use it very sparingly. I think I added it here because the search_engine parameter used to be optional, and I was having trouble tracing down all the possible lane calls, so I thought this was safer, but didn't think of the possible kwargs issues.

[jonathangreen]

Looks good as is, but if the parameter is getting set everywhere already, it probably makes sense to drop @inject decorator and the default for the arg so its more explicit:

def update_size(
    self, _db, search_engine: ExternalSearchIndex
):

[jonathangreen]

This one should probably go out as a hotfix once its merged. So we might want to wait on the production rollout until its merged.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1be90db) 90.04% compared to head (e48d168) 90.04%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
- Coverage   90.04%   90.04%   -0.01%     
==========================================
  Files         245      245              
  Lines       28167    28166       -1     
  Branches     6423     6422       -1     
==========================================
- Hits        25364    25363       -1     
  Misses       1855     1855              
  Partials      948      948              

Flag	Coverage Δ
Api	75.78% <100.00%> (-0.01%)	⬇️
Core	59.77% <25.00%> (-0.01%)	⬇️
migration	26.87% <25.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.