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

Issue with update of provider config field AllowPermanentAccess #234

Closed
bsushmith opened this issue Jul 29, 2022 · 2 comments · Fixed by #238
Closed

Issue with update of provider config field AllowPermanentAccess #234

bsushmith opened this issue Jul 29, 2022 · 2 comments · Fixed by #238
Assignees
Labels
bug Something isn't working provider

Comments

@bsushmith
Copy link
Member

bsushmith commented Jul 29, 2022

Description

On update of provider, we use merge of existing value in DB and new updated value, to autofill the empty values and take the latest provider value.

If we have a provider record in DB with Provider.Config.Appeal.AllowPermanentAccess value as true already, and we update this field to false in new the updated provider record - because of the bug in the version of mergo that we use, the field will always remain true in DB.

mergo issue: darccio/mergo#166

To Reproduce

  • Insert a provider record in DB with Provider.Config.Appeal.AllowPermanentAccess value as true
  • Update the same provider record with Provider.Config.Appeal.AllowPermanentAccess value as false
  • Check the provider record; The value would still be true instead of false

Expected behavior
The field should be updated to whatever's the value in the latest provider record.

Additional context

  • Version - latest main
  • We use the same merge functionality for update resource also.
  • Need to check if the latest version of mergo lib solves this, if not need to look for alternative fixes in code.
@bsushmith bsushmith added bug Something isn't working provider labels Jul 29, 2022
@bsushmith
Copy link
Member Author

bsushmith commented Jul 30, 2022

I see that this is not just for this bool field, but also for other fields.

Labels(map[string]string) are also merged instead of simple update and both AllowPermanentAccess and AllowActiveAccessExtensionIn are taken from existing provider values rather than the incoming update.

Is this desired? If not, we should relook at how we use the merge library to autofill the missing values, and create our own provider struct manually for all fields taking in the new fields and with default empty values for missing fields.

IMO, I don't think we should merge at all, we should be taking in the latest values always.

WDYT @rahmatrhd @ravisuhag @singhvikash11

@mabdh
Copy link
Member

mabdh commented Aug 1, 2022

If it is PUT API I think the expectation should be: it should update the entire record with the one passed in the API so the latest value will be stored. If we desire to change partial fields in the record, that should be with PATCH API.

@bsushmith bsushmith self-assigned this Aug 1, 2022
lifosmin pushed a commit to lifosmin/guardian that referenced this issue Aug 31, 2023
* feat: add create alert per namespace api

* feat(siren): distinguish create alert request response
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working provider
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants