Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ELE-716] Report fails to generate if owner is configured as dict (exposures format) #827

Closed
Maayan-s opened this issue Apr 24, 2023 · 3 comments · Fixed by #847
Closed
Labels
Bug Something isn't working Triage 👀

Comments

@Maayan-s
Copy link
Contributor

Maayan-s commented Apr 24, 2023

Describe the bug
We expect a string or list of strings in the owner field for sources.
For other format, we fail.
Example: {"name": "Maayan Salom", "email": "maayan@elementary-data.com"}

To Reproduce
Configure source owner like this:

owner:
  name: name
  email: email@gmail.com

Expected behavior
We shouldn't crush on it, even if we don't support thr format.

Screenshots
image

ELE-716

@Maayan-s Maayan-s added Bug Something isn't working Triage 👀 labels Apr 24, 2023
@Maayan-s Maayan-s changed the title Report fails to generate if owner is configured as dict (exposures format) [ELE-716] Report fails to generate if owner is configured as dict (exposures format) Apr 24, 2023
@manulpatel
Copy link
Contributor

Hi! @Maayan-s. I would be happy to fix this issue. To display the owner as it is in case of dict, we need to pass an exception for Error 1 , here.

What do you suggest?

@Maayan-s
Copy link
Contributor Author

Thank you @manulpatel !
@IDoneShaveIt could you please take a look and let @manulpatel know if this is the way to go for fixing this?

@IDoneShaveIt
Copy link
Contributor

Hey @manulpatel, sorry for the late reply!
I think that handling the error at the DataMonitoringReport.get_report_data won't do the trick because the error that we are occurring might break the rest of the report data generation.

The error that we get is a pydantic error where we pass data that does not match a pydantic schema (in our case we pass a dict when the schema expects a list of strings).

The schema that is throwing the error is ArtifactSchema.
It is using a method called _load_var_to_list at the load_owners validator.

We need to edit _load_var_to_list to handle our case when our owner is a string of a dict '{"name": "text", "email": "text"}'.
To do so, we need to check the output of try_load_json and if it's a dict, we should do one of the 2:

  1. Return an empty list (because we don't know what are the keys of the dict)
  2. json.dumps the dict and return it as an item of a list (that way we will return the whole data of the dict)

I prefer option 2 because that way we won't "lose" data.

@manulpatel thank you again for willing to fix this issue! I am here to help with any question, and hope this made the error clearer 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Triage 👀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants