fix(commands): it is now impossible to create a study with an empty id #1531
Conversation
| @@ -334,6 +334,11 @@ def remove_layer(self, study: RawStudy, layer_id: str) -> None: | |||
| def create_area( | |||
| self, study: Study, area_creation_info: AreaCreationDTO | |||
| ) -> AreaInfoDTO: | |||
| area_id = transform_name_to_id(area_creation_info.name) | |||
| if area_id == "": | |||
| raise ValueError( | |||
There was a problem hiding this comment.
Si tu lèves une exception de type ValueError, cela va causer une erreur 500, ce qui ne serait pas très utile. Il serait préférable d'utiliser une exception spécifique comme ValidationError (de Pydantic), qui générerait une erreur 422. En général, il est préférable d'éviter d'utiliser des exceptions de la bibliothèque standard et plutôt utiliser une sous-classe de Exception. Pour les managers, nous avons pris l'habitude de lever des exceptions de type HTTPException. Étant donné que les managers ne sont utilisés que par les fonctions qui implémentent un endpoint, utiliser une sous-classe de HTTPException est tout à fait acceptable, donc nous pouvons partir là-dessus.
| @@ -334,6 +334,11 @@ def remove_layer(self, study: RawStudy, layer_id: str) -> None: | |||
| def create_area( | |||
| self, study: Study, area_creation_info: AreaCreationDTO | |||
| ) -> AreaInfoDTO: | |||
| area_id = transform_name_to_id(area_creation_info.name) | |||
| if area_id == "": | |||
There was a problem hiding this comment.
Je recommande if not area_id.
| @@ -59,14 +59,21 @@ def _apply_config( | |||
| raise ValueError() | |||
|
|
|||
| area_id = transform_name_to_id(self.area_name) | |||
|
|
|||
| if area_id == "": | |||
There was a problem hiding this comment.
À remplacer par if not area_id
| type=EventType.STUDY_DATA_EDITED, | ||
| payload=study.to_json_summary(), | ||
| permissions=PermissionInfo.from_study(study), | ||
| try: |
There was a problem hiding this comment.
Il n'est pas vraiment nécessaire d'utiliser un gestionnaire d'exception ici uniquement pour enregistrer un message d'erreur, car les exceptions non gérées sont déjà signalées lors de la gestion de l'erreur 500.
| payload=study.to_json_summary(), | ||
| permissions=PermissionInfo.from_study(study), | ||
| try: | ||
| new_area = self.areas.create_area(study, area_creation_dto) |
There was a problem hiding this comment.
À mon avis, pour vérifier le format du nom de la zone, on pourrait utiliser une regex, par exemple :
class AreaCreationDTO(BaseModel):
name: Field(regex=r"[\w(),&-][ \w(),&-]*")
type: AreaType
metadata: Optional[PatchArea]
set: Optional[List[str]]En procédant ainsi, la validation se ferra directement au niveau du end point.
| @@ -241,6 +241,13 @@ def test_area_crud( | |||
| ], | |||
| RequestParameters(DEFAULT_ADMIN_USER), | |||
| ) | |||
| with pytest.raises( | |||
| ValueError, | |||
There was a problem hiding this comment.
Proposer une autre exception.
Bug Fixes
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
No description provided.