Reviews & Feedback

[zepich]

Last response from @Digi92 in 303:

Hi @zepich and @StrangerGithuber,

for a first/second (sorry for my last late reply) development version, this is really impressive.
I especially like the ability to create rule package from multiple sources.

I took a look at the project and tried it out.
Generally, a lot of things already work very well.

However, I have a few remarks:

• The output folder must be created manually before execution, as it is currently not created automatically. I think it would be useful to implement the automatic creation of the folder if it does not yet exist.
• I think it would not be bad to have the possibility to generate a file in the format filename.yaml.uuidIndex from an existing rule package.
• In the current filename.yaml.uuidIndex format, the name field of the rule is used. Wouldn't it make more sense to use the identifier field instead? (See: src/Context/ContextInterface.php:buildIdentifier).
• For the Web type, I would suggest adding an optional function that allows hash files to be downloaded so that their hash value (e.g. MD5) can be used to check the integrity of the actual file.
• Wouldn't it be better to define a PHP 8 enum for RuleItemContext->Type that contains all valid types? This would not only simplify type validation. It would also prevent incorrect rules from being created in the first place.

Kind regards,
Digi92

[zepich]

Hi @Digi92

Thank you very much for your feedback! I'm very happy to read your message! And, importantly, it's not your fault regarding the two versions! I wasn't happy with my first version since it was not as flexible as I planned... But when I started, I had so many ideas in my head, and I didn't want to wait... I was pumped, and I needed to do it... :)

Regarding your remarks:

1. Output dir: Yes, absolutely! I manually created the input and output directories when I started the project, so I never had any issues. After committing the version and trying it out in a new directory, I found that problem as well... ;)
2. I think it would be no problem to do that, but on the other side: Why? The bad thing about forgetting UUIDs from my side is that we cannot map the rule in a submission to the correct rule item after the UUID has changed in the rule package. So, if you use the RPG and it generates a new UUID for a rule, you have this issue once, and after two weeks, the submission will match that rule item again. Or why does it make sense to have such a command? In which use case would it be helpful?
3. Yes, but: We do not have the validation right now. In my idea, the identifier was not planned as required (since it's not used in the rule package); the type and the name are required (for the rule package). But if we declare the identifier for every rule as required, we can use the identifier instead of the name since the identifier should change less than the name.
4. Yes, absolutely. I saw that code in the code you shared with us and wanted to add it, but I simply forgot it. I would add it at least for MD5, SHA1, and SHA2 since these are the normal algorithms for such file hashes.
5. Yes. In general, I did not add a lot of code regarding validation. I don't know right now what the best approach for the validation is. First, I thought we could use YAML schema - but there is no such thing. You could use JSON schema for YAML validation, but yeah... With our flexibility in RPG, a JSON schema would limit the possibility of customizing it. Another option from my perspective is to build the schema with the configuration tree syntax from Symfony - if we can use that for our case. But that's also more complicated, especially if you simply want to define your own type or processor. So, the best option, from my perspective, is to build the expected data structure in a simple array. The array would be extendable very easily. Additionally, every class/object can have its own definition for the validation part. Or what do you think? Do you have a better idea?

Kind regards,

zepich

[Digi92]

Hi @zepich,
i know that feeling all too well :)

Regarding your answers:

1. My thought is that with particularly large rule packages, the database load for mosparo would be unnecessarily high since existing rules are not updated but deleted and newly created. In addition, I can well imagine that the filename.yaml.uuidIndex file is more easily lost than the actual rule package or in the case of rule packages from the Internet, does not exist at all.
   (Especially with the possible option of merging several existing rule packages into a new one, I wouldn't think it would be a bad idea.)

2. I'm a bit torn on this.
   On one hand, I did prefer not to introduce new required fields and potentially change the existing structure. On the other hand, using a proper identifier field instead of a name field as an ID seems like the better long-term approach, as names are more likely to change over time.

3. That's a difficult question.
   I agree that, for now, the best solution would probably be a simple array, where each type/processor checks at the beginning whether its options are valid.
   Using an array keeps things flexible and easy to extend without introducing unnecessary complexity.
   So, I think your approach makes sense to keeping validation lightweight while ensuring that every type or processor defines its own validation rules.

Kind regards,
Digi92

[zepich]

Hi @Digi92

Thank you very much for your feedback.

So, number 1 and 2 are connected in my eyes.

If we add a tool to create the UUID index from an existing rule package, but on the other side, we use the identifier to identify the UUID in the UUID index, we would have to add a tool that exports exactly one rule from the existing rule package based on the name of the rule. Something like that:

php bin/console rpg:export-uuid-index --name "Name of the rule" --identifier "rpg-profile-identifier" --rule-package-file ./my-existing-rule-package.json

(--name to identify the rule in the existing rule package, --identifier to define the identifier for the UUID index)

Otherwise, we do not have the identifier combined with the existing name. Instead of the name, we could, of course, use the UUID, but in both scenarios, you have to specify the rule's identifying value in the existing rule package.

But what if we use the UUID as an identifier? In the RPG profile file, we add a property 'uuid', with which you define the rule's UUID. Since the UUID is a unique identifier, the identifier property can be removed.

Importing UUIDs from an existing rule package is no problem because the rule's UUID is defined in the RPG profile file and the existing rule package, so we can match that and have our index.

The negative side of this is that you have to create a UUID when you add a new input to the profile. Since we discussed the idea of adding a command to create a profile file, we can generate the UUID automatically. If there is a Web UI, the UI can, of course, also generate the UUIDs automatically. Additionally, we could add a simple tool to generate a new UUID (php bin/console rpg:generate-uuid).

We do not invent a new field that you have to fill out when using the UUID. Of course, the UUID is not required in the profile file right now, but it is required for the rule package.

What do you think about that?

Kind regards,

zepich

[Digi92]

Hi @zepich,
i think we should go with the UUID.
It makes sense to have them as a mandatory field in the profile as they are already in the rule package.
I don't think it is a problem that the UUID would have to be created in the profile, if we want to add a generator and/or a UI in the future.

Regarding the "rpg:generate-uuid" command, I think a reference to a simple online tool such as “https://www.uuidgenerator.net/” would do the job.

The command "rpg:export-uuid-index" itself looks so good to me.

Kind regards,
Digi92