-
Notifications
You must be signed in to change notification settings - Fork 91
Fix backward compatibility for renamed exception classes (issue #220) #222
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
Conversation
Resolves GitHub issue #220 by adding backward compatibility aliases for exception classes that were renamed in commit fdd69b1 to follow PEP8 naming conventions. Changes: - Add RTSLibALUANotSupported alias for RTSLibALUANotSupportedError - Add RTSLibNotInCFS alias for RTSLibNotInCFSError - Export both old and new names in __all__ - Add comprehensive unit tests for backward compatibility This restores compatibility with external projects like targetd that depend on the original exception names while maintaining the new PEP8-compliant names.
Allow S101 (assert detection) and PLC0415 (import at top-level) rules to be ignored in test files where they are standard practice.
71074bf to
e8dc9ce
Compare
|
This one needs more work. |
| raise RTSLibError("Invalid parent Target") | ||
|
|
||
| self._path = "%s/tpgt_%d" % (self.parent_target.path, self.tag) | ||
| self._path = f"{self.parent_target.path}/tpgt_{self.tag}" |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
CFSNode
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The recommended approach is to avoid direct attribute overwriting. In this case:
- Update the
CFSNodesuperclass (if possible—given only what we see here) so that_pathis either passed as a parameter or defined only in the superclass from a property/setter. - Since we cannot modify the superclass (code not shown), and this method is in a subclass, the best fix is to:
- Instead of setting
self._pathdirectly in the subclass, pass the full path as a parameter to the superclass constructor viasuper().__init__(), if supported and if the constructor signature allows. - If the superclass does not accept a path as a constructor argument, then define a new uniquely-named attribute in the subclass (e.g.,
self._tpg_path), and use that in this subclass instead ofself._path. - Refactor all uses of
self._pathin this subclass toself._tpg_path.
- Instead of setting
Specifically:
- In the code block, replace assignment to
self._path = ...withself._tpg_path = .... - Update subsequent code in this method (and elsewhere in this class, if needed) that references
self._pathtoself._tpg_path. - Do not assign directly to
self._pathin the subclass. - No additional imports or new methods are needed.
-
Copy modified line R200 -
Copy modified line R202
| @@ -197,9 +197,9 @@ | ||
| else: | ||
| raise RTSLibError("Invalid parent Target") | ||
|
|
||
| self._path = f"{self.parent_target.path}/tpgt_{self.tag}" | ||
| self._tpg_path = f"{self.parent_target.path}/tpgt_{self.tag}" | ||
|
|
||
| path = Path(self._path) | ||
| path = Path(self._tpg_path) | ||
| target_path = Path(self.parent_target.path) | ||
| if not self.has_feature('tpgts') and not path.is_dir(): | ||
| for filename in target_path.iterdir(): |
| self._lun = lun | ||
|
|
||
| self._path = "%s/lun/lun_%d" % (self.parent_tpg.path, self.lun) | ||
| self._path = f"{self.parent_tpg.path}/lun/lun_{self.lun}" |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
CFSNode
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we need to avoid directly overwriting the _path attribute in the subclass after it has been set by the superclass. The best approach is to ensure that the intended value for _path is set centrally during initialization, ideally through a parameter passed to the superclass's __init__ method if possible.
Assuming that CFSNode currently sets self._path in its __init__, the code can be refactored as follows:
- Alter
CFSNode.__init__to accept apathparameter (defaulting toNoneif necessary) and setself._path = pathif provided. - In the
LUN's__init__, compute the intended path string before callingsuper().__init__(), and pass it as a parameter, so the superclass sets_path, and no overwrite is needed.
All changes should be limited to the LUN class definition within rtslib/target.py, since that's the code provided.
If the superclass already accepts parameters for custom path initialization, use that parameter as intended. Otherwise, refactor as above, within the visibility of the provided code. If that's not possible, then the subclass should use a different attribute name (e.g., _lun_path) to avoid shadowing the superclass's _path.
-
Copy modified lines R529-R530
| @@ -507,8 +507,6 @@ | ||
| @type alias: string | ||
| @return: A LUN object. | ||
| ''' | ||
| super().__init__() | ||
|
|
||
| if isinstance(parent_tpg, TPG): | ||
| self._parent_tpg = parent_tpg | ||
| else: | ||
| @@ -528,9 +526,9 @@ | ||
| raise RTSLibError(f"LUN must be 0 to {self.MAX_TARGET_LUN}") | ||
|
|
||
| self._lun = lun | ||
| lun_path = f"{self.parent_tpg.path}/lun/lun_{self.lun}" | ||
| super().__init__(path=lun_path) | ||
|
|
||
| self._path = f"{self.parent_tpg.path}/lun/lun_{self.lun}" | ||
|
|
||
| if storage_object is None and alias is not None: | ||
| raise RTSLibError("The alias parameter has no meaning " | ||
| "without the storage_object parameter") |
|
|
||
| self._path = "%s/np/%s:%d" \ | ||
| % (self.parent_tpg.path, self.ip_address, self.port) | ||
| self._path = f"{self.parent_tpg.path}/np/{self.ip_address}:{self.port}" |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
CFSNode
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best way to fix this problem is to avoid reassigning the _path attribute directly in the subclass if it is already set in the superclass. Instead, _path should be managed centrally in the superclass, ideally via an __init__ argument if the value needs to be set in a subclass-specific way, or through a property if dynamic calculation is needed.
General Approach:
- Refactor the superclass (
CFSNode) to accept_pathas an argument to its__init__method. - In the subclass's
__init__, calculate the appropriate path string first, then pass it to the superclass viasuper().__init__(_path=calculated_path, ...). - Remove all direct assignments to
self._pathin the subclass.
What/where to change:
- In the code for
NetworkPortal(or the specific subclass), replace the direct assignment ofself._path = ...with a calculation beforesuper().__init__, and pass the result into the superclass's constructor. - Update the call to
super().__init__()on line 834 to pass in the calculated_path.
NOTE: We are only allowed to update code seen in the snippet. As such, the fix will focus on moving self._path computation before super() and passing it as an argument, and assume that CFSNode can accept this (otherwise we'd need to update CFSNode, which is out of scope here).
-
Copy modified line R846 -
Copy modified lines R848-R849
| @@ -831,8 +831,6 @@ | ||
| @type mode:string | ||
| @return: A NetworkPortal object. | ||
| ''' | ||
| super().__init__() | ||
|
|
||
| self._ip_address = str(ip_address) | ||
|
|
||
| try: | ||
| @@ -845,8 +843,10 @@ | ||
| else: | ||
| raise RTSLibError("Invalid parent TPG") | ||
|
|
||
| self._path = f"{self.parent_tpg.path}/np/{self.ip_address}:{self.port}" | ||
| _path = f"{self.parent_tpg.path}/np/{self.ip_address}:{self.port}" | ||
|
|
||
| super().__init__(_path=_path) | ||
|
|
||
| try: | ||
| self._create_in_cfs_ine(mode) | ||
| except OSError as msg: |
| raise RTSLibError("Mapped LUN must be >= 0") | ||
|
|
||
| self._path = "%s/lun_%d" % (self.parent_nodeacl.path, self.mapped_lun) | ||
| self._path = f"{self.parent_nodeacl.path}/lun_{self.mapped_lun}" |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
CFSNode
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, ensure that the _path attribute is not directly overwritten in the subclass after being set in the superclass. Instead, pass the correct _path value up to the superclass __init__ method, delegating initialization properly.
- Determine where
CFSNode.__init__()sets_pathand refactor its constructor to receive_pathas an optional argument, defaulting to its previous logic if not provided. - In the subclass's
__init__, compute the intended_pathbefore callingsuper().__init__, and pass it as an argument to the superclass constructor. - Remove the direct subclass assignment to
self._path; all initialization should go through the superclass.
You only need to modify the area in rtslib/target.py that defines the subclass and the construction of _path in its __init__ method. If the superclass is not shown here, you can only edit what is visible in the subclass.
-
Copy modified lines R1192-R1193 -
Copy modified line R1209
| @@ -1189,7 +1189,8 @@ | ||
| @type write_protect: bool | ||
| ''' | ||
|
|
||
| super().__init__() | ||
| path = f"{parent_nodeacl.path}/lun_{int(mapped_lun)}" | ||
| super().__init__(_path=path) | ||
|
|
||
| if not isinstance(parent_nodeacl, NodeACL): | ||
| raise RTSLibError("The parent_nodeacl parameter must be a NodeACL object") | ||
| @@ -1205,7 +1206,7 @@ | ||
| if self._mapped_lun < 0: | ||
| raise RTSLibError("Mapped LUN must be >= 0") | ||
|
|
||
| self._path = f"{self.parent_nodeacl.path}/lun_{self.mapped_lun}" | ||
| # self._path assignment moved into superclass constructor | ||
|
|
||
| if tpg_lun is None and write_protect is not None: | ||
| raise RTSLibError("The write_protect parameter has no " |
Summary
Changes Made
rtslib/utils.py:RTSLibALUANotSupported = RTSLibALUANotSupportedErrorRTSLibNotInCFS = RTSLibNotInCFSErrorrtslib/__init__.pyto export both old and new exception namestests/directory with comprehensive unit tests for backward compatibilityTest Plan
Related
Addresses #220