NextCenturyCorporation / itm-scenario-validator

1 stars 0 forks source link

Option 2 - validate based on api yaml #2

Closed kaitlyn-sharo closed 9 months ago

kaitlyn-sharo commented 9 months ago

Different from the first PR, this one has a mock yaml file for the api based on this

The rest works approximately the same. We need to discuss which way is preferred moving forward.

Use validator2.py to test

nextcen-dgemoets commented 9 months ago

It appears that the code requires there to be at least one required field, but I don't believe this will always be the case. E.g., tagging, where either reference is required or the other three fields are required. Also, condition: no one type of condition is required.

kaitlyn-sharo commented 9 months ago

It appears that the code requires there to be at least one required field, but I don't believe this will always be the case. E.g., tagging, where either reference is required or the other three fields are required. Also, condition: no one type of condition is required.

I noticed this while I was coding but forgot to go back and fix it. Updated now.

nextcen-dgemoets commented 9 months ago

Current issues:

Things that don't need to be addressed in this PR:

kaitlyn-sharo commented 9 months ago

Current issues:

  • Getting a crash, I think because Threat has no properties, only additionalProperties;
  • I think there's an issue with strings that look like integers (see kdma_associations in actions keyword); and
  • Need to check floats-- I updated representation in YAML and tried to address code, but the code changes might be incomplete.

Things that don't need to be addressed in this PR:

  • rank and rank_title need correct controlled vocabulary;
  • Need to address how we want to be able to specify any changes in state at the scene level, but not have to specify things that haven't changed;
  • Adding numeric value ranges to YAML and validating in tool;
  • Need to figure out if initial state will be in state property at top level, or it is specified in the 0th scene; and
  • Incorporating all changes from swagger.yaml in the development branch. Could do here or in another ticket.

Bugs are fixed and yes, you changed 'float' to 'number' in primitive types. That broke things. We can keep both, but the api could just specify ints vs floats. It doesn't bother me to keep it as a number. A string is validated now if the content is an int or float, and I did some extra checking for the additionalProperties object types and enums, since those formats in the api yaml broke things. There's probably some duplicated code and a few counting things and printing things and commenting things to clean up, so I'll start with that on Monday just to tighten it up and prepare for the next task. I also plan to finish making all of the enums into their own separate section. I wonder how you feel about keeping enums separate from the schemas?

nextcen-dgemoets commented 9 months ago

Bugs are fixed and yes, you changed 'float' to 'number' in primitive types. That broke things. We can keep both, but the api could just specify ints vs floats. It doesn't bother me to keep it as a number. A string is validated now if the content is an int or float, and I did some extra checking for the additionalProperties object types and enums, since those formats in the api yaml broke things. There's probably some duplicated code and a few counting things and printing things and commenting things to clean up, so I'll start with that on Monday just to tighten it up and prepare for the next task. I also plan to finish making all of the enums into their own separate section. I wonder how you feel about keeping enums separate from the schemas?

I changed float to number because float isn't a valid type in swagger (as reported by https://editor.swagger.io/). I knew there were implications in the code that I hadn't addressed-- thank you for addressing them. :) Although I think we can remove 'float': float from the PRIMITIVE_TYPE_MAP.

The kdma_association property was copied from TA1, and they had them as strings. That said, a number/float seems to be better so I guess we can change them.

Keeping the enums separate from the schemas was necessary to validate controlled vocabulary in the server, but maybe this is no longer necessary if there's a validator tool, right? It creates quite a proliferation of model objects / code. These enum objects can also be used by TA2 clients, however, to ensure that they're sending good values at runtime. It's something we can discuss with Brian maybe.

kaitlyn-sharo commented 9 months ago

Looks really great. I couldn't find any bugs.

The only thing that I might have expected the validator to do is detect duplicate keys-- if in a given level you specify the same key twice. It currently silently ignores the first value. This may be a function of the yaml package and not the validator. Anyway, it could be another ticket for someday.

The other thing we discussed which is specifically out of scope for this ticket is to configure minimum and/or maximum values in the yaml and validate them in the tool.

That's a great point. I actually used to have that (maybe only in option 1?). It's easy to add in a check, but you're right - it seems the yaml package is the one ignoring it. My code editor yells at me for having a duplicate anyways, so hopefully TA1's will do the same...I will make a ticket for looking into how to find these duplicates