Fixes #23688: Import archive must refuse yaml technique with mismatch directory and id #5153
Conversation
|
PR updated with a new commit |
| def checkTechniqueExistSameCat() = { | ||
| techInfo.techniquesCategory.collectFirst { | ||
| case (TechniqueId(name, _), catId) if (name.value == techArchive.technique.id.name.value) => catId | ||
| } match { | ||
| case None => // technique not present, ok | ||
| ZIO.unit | ||
| } else { | ||
| Inconsistency( | ||
| s"Technique '${techArchive.technique.id.serialize}' from archive has category '${archive}' but a " + | ||
| s"technique with that name already exists in category '${existing}': it must be imported in the " + | ||
| s"same category, please update your archive." | ||
| ).fail | ||
| } | ||
| case Some(catId) => | ||
| val existing = catId.getPathFromRoot.map(_.value).tail.mkString("/") | ||
| val archive = techArchive.category.mkString("/") | ||
| if (existing == archive) { | ||
| ZIO.unit | ||
| } else { | ||
| Inconsistency( | ||
| s"Technique '${techArchive.technique.id.serialize}' from archive has category '${archive}' but a " + | ||
| s"technique with that name already exists in category '${existing}': it must be imported in the " + | ||
| s"same category, please update your archive." | ||
| ).fail | ||
| } | ||
| } | ||
| } | ||
|
|
||
| checkTechniqueExistSameCat() | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of wrapping the method body with a more descriptive name, shouldn't it be better renaming checkTechnique to checkTechniqueExistsSameCat then ?
Or simply comment on the method ? There already is a line comment but inside the method...
There was a problem hiding this comment.
I change that b/c I though I would be able to just chain check here, so the shape would be:
def checkTechnique(techInfo: TechniquesInfo, techArchive: TechniqueArchive): IOResult[Unit] = {
def checkTechniqueExistSameCat() = { ... }
def otherCheck() = {... }
checkTechniqueExistSameCat() *> otherCheck()
}
I was not able to do it b/c that chek need to be done before hand, and there's no simple way to factor out all checks at the same place, but I prefer to let it that way, it will be more simple to add other checks latter on (and the cost in perf/compilation/whatever is negligible)
|
LGTM ! (apart from my comment on the method name) |
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
… directory and id
fanf
force-pushed
the
arch_23688/import_archive_must_refuse_yaml_technique_with_mismatch_directory_and_id
branch
from
5c4bc23
to
ef8e443
Compare
https://issues.rudder.io/issues/23688
Add a check that Technique ID from path pattern matches technique from descriptor for YAML and JSON case after parsing the descriptor. We already have everything, just need to test at the correct place.
The most part of the change is an unit test.