Skip to content

Conversation

@staudtMarius
Copy link
Member

Resolves #670

@staudtMarius staudtMarius added the enhancement New feature or request label May 11, 2023
@staudtMarius staudtMarius added this to the Version 3.1 milestone May 11, 2023
@staudtMarius staudtMarius self-assigned this May 11, 2023
@staudtMarius staudtMarius marked this pull request as ready for review June 7, 2023 11:42
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check out the SonarQube warnings. To solve them, it could be helpful to check out how org.slf4j.Logger can format log messages with supplied objects

@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented Jun 26, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/ie3-institute/PowerSystemDataModel/807.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/ie3-institute/PowerSystemDataModel/807.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

@sebastian-peter sebastian-peter modified the milestones: Version 3.1, Version 4.0 Jul 25, 2023
@sebastian-peter
Copy link
Member

sebastian-peter commented Jul 26, 2023

Ok I just realized we already have a validation in Factory#validateParameters, which is called by Factory#get(D), which is called everytime that any entity is parsed from some source. The problem with IdCoordinateSources is that these validations are called after some other things, like duplicate removal, happened, that already access the data and fail if some column is missing.

Looking at Factory.get() as a whole, I'd describe two main issues:

  1. validation is sometimes, like in the case of IdCoordinateSource, called too late
  2. validation is executed for every single data point parsed from Factory.get(), which is pretty costly

So I think there's a way to fix both at the same time: Validate column names once during initialization (and early enough). Opened up a new issue here: #849

With regards to this PR: We could merge it to fix the problem in IdCoordinateSource, but we then have to remember removing this again once validation is fixed in general.

@staudtMarius @t-ober @danielfeismann

…source-with-invalid-column-names

# Conflicts:
#	src/test/groovy/edu/ie3/datamodel/io/factory/timeseries/CosmoIdCoordinateFactoryTest.groovy
#	src/test/groovy/edu/ie3/datamodel/io/factory/timeseries/IconIdCoordinateFactoryTest.groovy
@sebastian-peter sebastian-peter modified the milestones: Version 4.1, Version 4.2 Nov 2, 2023
@staudtMarius
Copy link
Member Author

Will be resolved in #926.

@staudtMarius staudtMarius deleted the ms/#670-enhance-error-handling-of-coordinate-source-with-invalid-column-names branch November 10, 2023 10:45
@sebastian-peter sebastian-peter modified the milestones: Version 4.2, Version 5.0 Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhance error handling of coordinate source with invalid column names

3 participants