fix(links): endpoint links create #2146
Conversation
|
I see that there are still 68 tests that fail (probably all for the same reason) so I'll let you fix that before reviewing your work :) |
skamril
changed the title
| color: str | ||
| width: float | ||
| style: str | ||
| class LinkInfoDTOBase(AreaInfo, LinkInfoProperties): |
There was a problem hiding this comment.
Class AreaInfo seems not used elsewhere, it should be defined here.
Not a big fan of using inheritance that much, areas could also got just in the definition of those 2 classes. But Ok to leave it this way.
There was a problem hiding this comment.
I had troubles with some tests when removing the area value, especially commands tests. I left it that way
|
|
||
| class LinkInfoProperties820(LinkInfoProperties): | ||
| filter_synthesis: t.Optional[str] = None | ||
| filter_year_by_year: t.Optional[str] = None |
There was a problem hiding this comment.
I think the best approach here, for both fields, would be to use an annotated type with a custom serializer and validation method.
This way, the fields can have their "real" type which is a List[FilterOption] while still being serialized and parsed as a str.
This will allow to re-use this annotated type in other places if needed.
There was a problem hiding this comment.
I added an annotated field that performs validation and serialization but the methods are not reusable
antarest/study/storage/variantstudy/model/command/create_link.py
Outdated
Show resolved
Hide resolved
Updated the structure of LinkInfoDTO classes to use Pydantic's ConfigDict for configuration and streamlined validation logic. Removed unnecessary imports and redundant configurations, while ensuring filter fields are checked appropriately for different study versions.
Use a centralized FILTER_VALUES list for filter_synthesis and filter_year_by_year in LinkInfoProperties820. This change simplifies code maintenance and ensures consistency across filter validations.
Replaced filter fields with a comma-separated list of enums for better validation and clarity. This enhances type safety and reduces the risk of invalid filter values being used. The previous string-based validation approach has been removed in favor of utility functions for converting and serializing enum lists.
Ensure code readability and consistency by adding a newline before the definition of the `LinkInfoProperties820` class. This change improves the visual separation and organization of the code.
…-creation # Conflicts: # antarest/study/business/link_management.py # antarest/study/business/table_mode_management.py
| model_config = ConfigDict(alias_generator=to_kebab_case, populate_by_name=True) | ||
|
|
||
|
|
||
| comma_separated_enum_list = Annotated[ |
There was a problem hiding this comment.
Nice!
I think we should name it following type conventions, in CamelCase, for example FilterOptionList
There was a problem hiding this comment.
Also we can move it to the same module where FilterOption is defined, because it's likely that we will want to re-use it where FilterOption is used (for example in LinkProperties etc, but this can be done in a future work, I don't know if it will have more impacts).
| comma_separated_enum_list = Annotated[ | ||
| t.List[FilterOption], | ||
| BeforeValidator(lambda x: validate_filters(x, FilterOption)), | ||
| PlainSerializer(lambda x: join_with_comma(x)), |
There was a problem hiding this comment.
detail: I think the lambda is not needed:
PlainSerializer(join_with_comma),|
|
||
|
|
||
| def validate_filters( | ||
| filter_value: Union[List[FilterOption], str], enum_cls: t.Type[FilterOption] |
There was a problem hiding this comment.
The function only works for FitlerOption class, so I don't think the second argument is useful ?
It would be if we wanted to use this function for other enum types, but here we have hard coded messages, so I don't think we will do it.
There was a problem hiding this comment.
Note:
I found out there are similar methods in antarest/study/storage/rawstudy/model/filesystem/config/field_validators.py, we should try to clean this code duplication at some point (we can create a specific task for this if not in that PR)
|
|
||
| comma_separated_enum_list = Annotated[ | ||
| t.List[FilterOption], | ||
| BeforeValidator(lambda x: validate_filters(x, FilterOption)), |
There was a problem hiding this comment.
detail: if we remove the second argument, lambda is not needed anymore
| @@ -26,22 +44,76 @@ | |||
| from antarest.study.storage.variantstudy.model.command_listener.command_listener import ICommandListener | |||
| from antarest.study.storage.variantstudy.model.model import CommandDTO | |||
|
|
|||
| DEFAULT_COLOR = 112 | |||
| FILTER_VALUES: t.List[FilterOption] = [ | |||
There was a problem hiding this comment.
maybe DEFAULT_FILTERS for homogeneity with the above var? indeed it's only used as the default value
| @@ -98,51 +170,6 @@ def _create_link_in_config(self, area_from: str, area_to: str, study_data: FileS | |||
| ], | |||
| ) | |||
|
|
|||
| @staticmethod | |||
| def generate_link_properties(parameters: JSON) -> JSON: | |||
| else: | ||
| properties = LinkInfoProperties.model_validate(self.parameters or {}) | ||
|
|
||
| link_property = properties.model_dump(mode="json", exclude={"area1", "area2"}, by_alias=True, exclude_none=True) |
There was a problem hiding this comment.
The excludes are not needed I think, the properties classes don't have information about areas
|
|
||
| properties: LinkInfoProperties | ||
| if StudyVersion.parse(version) >= STUDY_VERSION_8_2: | ||
| properties = LinkInfoProperties820.model_validate(self.parameters or {}) |
There was a problem hiding this comment.
Taking a step back about the architecture of our classes:
our logic is wrong here, the parameters were created by dumping a LinkInfoDTO, but we read it as a LinkInfoProperties. There is a kind of implicit conversion.
Instead, we should read it again as a DTO, and have an explicit conversion function to get a "properties" class that will be targetted at being serialized in the INI file (even if it only does a modeul_dump/model_validate to achieve that conversion).
| area_from = data["area_from"] | ||
| area_to = data["area_to"] | ||
|
|
||
| self.parameters = self.parameters or {} | ||
| link_property = CreateLink.generate_link_properties(self.parameters) | ||
|
|
||
| study_data.tree.save(link_property, ["input", "links", area_from, "properties", area_to]) |
There was a problem hiding this comment.
Again, taking a step back:
here we are serializing basically the content of a LinkInfoProperties into an ini file.
This seems like a duplication of the role of the LinkProperties class in antarest/study/storage/rawstudy/model/filesystem/config/links.py.
It looks like maybe the new classes you have created should go there and replace the old one.
OR, we should have a conversion function from the new classes to this one, and use it for serialization.
I am not sure of the best choice right now, we need to think about this from a general point of view, so that we use common principles everywhere.
I think we should use this work as an opportunity to clarify and agree about this design of classes with all the team.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14433191 | Triggered | Generic Password | b40ccf2 | webapp/src/utils/validation/string.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
| @@ -151,15 +151,14 @@ def get_areas( | |||
| ) | |||
| def get_links( | |||
| uuid: str, | |||
| with_ui: bool = False, | |||
| current_user: JWTUser = Depends(auth.get_current_user), | |||
| ) -> t.Any: | |||
| hurdles_cost: bool = False | ||
| loop_flow: bool = False | ||
| use_phase_shifter: bool = False | ||
| transmission_capacities: TransmissionCapacity = TransmissionCapacity.ENABLED |
There was a problem hiding this comment.
transmission_capacity in singular (there is an another enum named TransmissionCapacities)
There was a problem hiding this comment.
The issue is the field is written transmission-capacities inside the ini file :/
It's a shame that there's another field named the same way inside the generaldata.ini file ...
No description provided.