feature/remove-is-required-from-constant-validator

[isaacna]

Link to Relevant Issue

This pull request resolves #160

Description of Changes

Removing is_required and since we can use FireO's Required=True wherever applicable.

[codecov]

Codecov Report

Merging #163 (08978ac) into main (8743498) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
- Coverage   94.57%   94.56%   -0.01%     
==========================================
  Files          50       50              
  Lines        2560     2558       -2     
==========================================
- Hits         2421     2419       -2     
  Misses        139      139              

Impacted Files	Coverage Δ
cdp_backend/database/models.py	100.00% <ø> (ø)
cdp_backend/database/validators.py	84.48% <100.00%> (-0.52%)	⬇️
cdp_backend/tests/database/test_validators.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8743498...08978ac. Read the comment docs.

[evamaxfield]

This is making me question what the pattern is. Before I approve can you show me code that has a random FireO model with a field thats required fail to push with a None value?

Like I want to make sure that regardless of this change, that None values for required fields are still rejected from upload.

[isaacna]

Sure, here's an example code snippet I ran:

matter_status = MatterStatus()
matter_status.matter_ref = Matter.Example()
matter_status.status = None
upload_db_model(matter_status, config.google_credentials_file)

It will raise a FireO RequiredField exception:

Traceback (most recent call last):
  File "/Users/Isaac/Desktop/CDP/cdp-backend/cdp_backend/database/functions.py", line 107, in upload_db_model
    db_model = db_model.upsert(transaction=transaction, batch=batch)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/models/model.py", line 352, in upsert
    return self.save(transaction=transaction, batch=batch, merge=True)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/models/model.py", line 346, in save
    return self.__class__.collection.create(self, transaction, batch, merge, no_return, **self._get_fields())
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/managers/managers.py", line 230, in create
    return self.queryset.create(mutable_instance, transaction, batch, merge, no_return, **field_list)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/queries/query_set.py", line 56, in create
    return CreateQuery(self.model_cls, mutable_instance, no_return, **kwargs).exec(transaction_or_batch, merge)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/queries/create_query.py", line 158, in exec
    return query_wrapper.ModelWrapper.from_query_result(self.model, self._raw_exec(merge=merge))
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/queries/create_query.py", line 143, in _raw_exec
    ref.set(self._parse_field(), merge=merge)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/queries/create_query.py", line 97, in _parse_field
    fv = f.get_value(self.query.get(f.name))
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/fields/base_field.py", line 122, in get_value
    val = self.field_attribute.parse(val, ignore_required, ignore_default)
  File "/Users/Isaac/.local/share/virtualenvs/cdp-backend-9V6eR1KV/lib/python3.7/site-packages/fireo/fields/field_attribute.py", line 86, in parse
    raise RequiredField(f'"{self.field.__class__.__name__}" is required for model {self.field.model_cls} '
fireo.fields.errors.RequiredField: "TextField" is required for model <class 'cdp_backend.database.models.MatterStatus'> but received no default and no value.

Same thing happens when we don't set status at all

[isaacna]

If upload fails due to a required field not being present, do we want to fail on the pipeline or keep proceeding? If it's the latter, then I can update the PR to catch RequiredField for uploads.

I'm thinking it's one thing to proceed if validation fails, but if something like a required field is gone, it could be worth stopping the pipeline because that could mean something is wrong with the data source of the ingestion models

[evamaxfield]

Ahhh I think it depends on the case. I wonder if this was why we split them up initially? I will go through this PR and see which ones are needed and which one are truly optional

[evamaxfield]

Honestly. The more I look at this the more I am concerned I am missing something. Like was there a reason we originally included this param??

Looking at the pipeline code: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/pipeline/event_gather_pipeline.py#L1133

If we were to merge this PR, that line wouldn't break it would just raise a different error correct?

BUT, looking at this: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/pipeline/event_gather_pipeline.py#L1413

I think this would break some stuff because passing None would now be allowed through the validator I think but would raise a requiredField error that isn't current caught?

[isaacna]

I don't think To had a specific reason, but may have just added it if he didn't know about the required FireO field:
#35

Looking at the pipeline code: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/pipeline/event_gather_pipeline.py#L1133

If we were to merge this PR, that line wouldn't break it would just raise a different error correct?

Yeah this wouldn't break, it'd just throw RequiredField instead of FieldValidationFailed.

BUT, looking at this: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/pipeline/event_gather_pipeline.py#L1413

I think this would break some stuff because passing None would now be allowed through the validator I think but would raise a requiredField error that isn't current caught?

So I tested this out, and technically there would be no change to the existing functionality. If you look at this PR. We used the FireO required field before, but even with Sung's changes, the FireO required check executes before the validation check for a given field.

So we're already (and always have been) throwing RequiredField if it's missing or None. If we want to pass instead of erroring out we'd have to catch RequiredField as well if None is passed. But I wasn't sure if we'd necessarily want to because if the error is RequiredField then that means the ingestion data is wrong

Btw, the FieldValidationFailed and RequiredField errors on FireO models aren't triggered until we attempt to perform a write operation with the db model

[isaacna]

So TLDR: the validator param is_required isn't even doing anything, we should be able to just remove it

[evamaxfield]

So I tested this out, and technically there would be no change to the existing functionality. If you look at this PR. We used the FireO required field before, but even with Sung's changes, the FireO required check executes before the validation check for a given field.

Oh. Well that alone has convinced me. Sounds good. Thanks for the additional comments and research.