Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
erp5 erp5
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Labels
    • Labels
  • Merge requests 139
    • Merge requests 139
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Operations
    • Operations
    • Environments
  • Analytics
    • Analytics
    • CI/CD
    • Repository
    • Value Stream
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Activity
  • Graph
  • Jobs
  • Commits
Collapse sidebar
  • nexedi
  • erp5erp5
  • Merge requests
  • !938

Merged
Created Sep 25, 2019 by Nicolas Wavrant@NicolasMaintainer

Verify parameters passed to category setters

  • Overview 11
  • Commits 4
  • Changes 11

This merge request aims to prevent programming errors by raising instead of silently doing nothing (which is a source of bugs).

Currently, if an object as category property for a category "category", 2 families of setters were created :

  1. The string setter, following the format _setCategory and taking a relative URL as the argument.
  2. The value setter, following the format _setCategoryValue and taking an object as the argument.

The issue is that developers may pass the wrong argument to one of these functions, having for consequences :

  1. For case (1), if an object is passed, the code would silently do nothing : nothing is set as relation, but the code doesn't fail. This is the worst case.
  2. For the second case, passing a relative URL to _setCategoryValue would "work" (in the meaning the relation is set to the correct object). This may sound like a feature, but in my opinion it is confusing given the way ERP5 developers apprehend these setters nowadays.

For case 1, a test is existing that an exception is raised, but due to coding error the feature disappeared and no one noticed : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/CMFCategory/tests/testCMFCategory.py#L810-815

For case 2, compatibility code exists in the underlying function _setValue : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/ERP5Type/Base.py#L1840-1842

In this MR, the exception caused by case (1) has been restored (so now it fails loudly).

Case (2) has been deprecated, in order to keep backward compatibility.

I have run the tests with the DeprecationWarning raising an error instead of just a warning, and fixed the code were the setters weren't used correctly.

Ideally, tests should always run with this DeprecationWarning being a real error so both cases crash loudly. This won't be part of this Merge Request.

Assignee
Assign to
Reviewer
Request review from
None
Milestone
None
Assign milestone
Time tracking
Source branch: setter-type-security
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7