Handle unsupported IIDM file versions with a better error message #3646
Conversation
Signed-off-by: Clement Philipot <[email protected]>
cphili
force-pushed
the
improve_unsupported_iidm_version_error_message
branch
from
f5cd228 to
0521a4d
Compare
Signed-off-by: Clement Philipot <[email protected]>
Signed-off-by: Clement Philipot <[email protected]>
| .orElseThrow(() -> new PowsyblException("Version " + version + " is not supported.")); | ||
| } | ||
|
|
||
| public static int compareVersions(String v1, String v2) { |
There was a problem hiding this comment.
Usually, we use v1.compareTo(v2) to compare versions. This method may not be needed (and therefore neither parseVersion(...)).
|
|
||
| if (!ns.isEmpty()) { | ||
| String xmlIidmVersion = ns.substring(ns.lastIndexOf('/') + 1); | ||
| if (IidmVersion.compareVersions(xmlIidmVersion, CURRENT_IIDM_VERSION.toString()) > 0) { |
There was a problem hiding this comment.
Instead of comparing the strings of the 2 versions, it would be safer to try to load the version corresponding to the namespace, and if it fails to throw the exception with your message (in which you can also add the version that was encountered).
There was a problem hiding this comment.
You can load the IidmVersion corresponding to a string with IidmVersion.of(String version, String separator).
It throws an exception containing version if no IidmVersion value is found (so the version is not supported). You can just catch this exception and create your exception (with your message untouched) using the caught exception as cause.
iidm/iidm-serde/src/main/java/com/powsybl/iidm/serde/XMLImporter.java
Outdated
Show resolved
Hide resolved
iidm/iidm-serde/src/test/java/com/powsybl/iidm/serde/XMLImporterTest.java
Outdated
Show resolved
Hide resolved
iidm/iidm-serde/src/test/java/com/powsybl/iidm/serde/XMLImporterTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Clement Philipot <[email protected]>
Signed-off-by: Clement Philipot <[email protected]>
| String[] currentIidmVersionSplit = CURRENT_IIDM_VERSION.toString().split("_"); | ||
| int unsupportedIidmVersionMajor = Integer.parseInt(currentIidmVersionSplit[1]); | ||
| int unsupportedIidmVersionMinor = Integer.parseInt(currentIidmVersionSplit[2]) + 1; | ||
| String unsupportedIidmVersion = String.format("%d.%d", unsupportedIidmVersionMajor, unsupportedIidmVersionMinor); |
There was a problem hiding this comment.
This code could be extracted in a private static method to be reused by unsupportedIidmVersionNamespaceURI().
| } | ||
|
|
||
| private boolean isValidNetworkRoot(XMLStreamReader xmlsr) throws XMLStreamException { | ||
| while (xmlsr.hasNext()) { |
There was a problem hiding this comment.
The comment that previously existed was useful:
| while (xmlsr.hasNext()) { | |
| // check the first root element is network and namespace is IIDM | |
| while (xmlsr.hasNext()) { |
| .filter(v -> version.equals(v.toString(separator))) | ||
| .findFirst() // there can only be 0 or exactly 1 match | ||
| .orElseThrow(() -> new PowsyblException("IIDM Version " + version + " is not supported. Max supported version: " | ||
| + CURRENT_IIDM_VERSION.toString("."))); |
There was a problem hiding this comment.
| + CURRENT_IIDM_VERSION.toString("."))); | |
| + CURRENT_IIDM_VERSION.toString(separator))); |
| if (!ns.isEmpty()) { | ||
| String version = ns.substring(ns.lastIndexOf('/') + 1); | ||
| if (!version.isEmpty()) { | ||
| IidmVersion.fromNamespaceURI(ns); |
There was a problem hiding this comment.
- It would be better to use
IidmVersion.of(...). - Comments should be added to explain the purpose of this line.
| IidmVersion.fromNamespaceURI(ns); | |
| // Try to load an IidmVersion from the version number. | |
| // If it is not supported, this will throw an exception giving details on the encountered version | |
| // and the maximum supported version. | |
| IidmVersion.of(version, "_"); |
| if (!ns.isEmpty()) { | ||
| String version = ns.substring(ns.lastIndexOf('/') + 1); | ||
| if (!version.isEmpty()) { | ||
| IidmVersion.fromNamespaceURI(ns); |
There was a problem hiding this comment.
Another problem here. You could not throw a exception here without a minimum of checks.
When a file is imported, in Importer.find(...), all the importer present in the class loader will be tested to see if one of them support the file format. I think that you can reach this line each time you import an XML file whose extension is .xml and whose first root element is network.
If you have a custom network file format with these characteristics, you may encountered the unsupported IIDM version exception depending on the order the importers are tested.
One control that can be added is on the known namespace prefixes. I suggest that you loop on IidmVersion.values() to retrieve their namespace prefixes (by this, I mean the part before the last'/') and to check that the one of the current namespace is equal to one of them. If it is indeed, you can check the version and raise an exception if it is unknown. Else, you'll have to ignore the check.
Signed-off-by: Clement Philipot <[email protected]>
Signed-off-by: Clement Philipot <[email protected]>
|
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
#3553
What kind of change does this PR introduce?
Make the error clearer when importing an IIDM file from an unsupported version.
What is the current behavior?
Currently, the error message "Unsupported file format or invalid file." is unclear.
What is the new behavior (if this is a feature change)?
The error message now states that version is not supported (maximum supported version: <IIDM_CURRENT_VERSION>).
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: