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

Problem removing role from global request settings #367

Closed
mafiu opened this issue May 10, 2017 · 2 comments
Closed

Problem removing role from global request settings #367

mafiu opened this issue May 10, 2017 · 2 comments
Milestone

Comments

@mafiu
Copy link

mafiu commented May 10, 2017

Dear @GUI,

I found that when you want to delete a role from a API Backends it's not removed and neither changed when you click on "Publish Changes"

To reproduce:

  1. Just create an API Backend
  2. Add or create a role in Global Request Settings
  3. Publish API

Then...
4. Remove the role added.
5. Publish API

And no change is made.

I'm using API-Umbrella 0.14.0 and also I've tested on 0.14.1

@GUI
Copy link
Member

GUI commented May 11, 2017

Thanks for catching this! Just to clarify, after reproducing this, it looks like you can remove roles if there's multiple roles present, but you can't remove the final role (so you can't completely remove the role requirements once there are any).

I'll try to get this fixed later this week.

GUI added a commit that referenced this issue May 15, 2017
This fixes various issues surrounding embedded array handling and not
being able to remove the last item. This was a regression in API
Umbrella v0.14.0, due to the Rails 4 upgrade and differences in how
Rails permitted params ignores array fields set to an empty array or
null by default.

This came up in #367 due to
not being able to remove the last required role from an API backend.
However, this also impacted other areas of the admin and led to some
better standardization, cleanup, and testing for other embedded array
handling.

- Fix removing the last role from an API backend not working.
- Fix removing the last role from a user actually resulting in a role
  name of "" being assigned.
- Fix not being able to remove the last group from an admin user.
- Fix a bug with API backend throwing a Mongo error during saves if a
  sub-setting record was removed at the same time other sub-setting
  records were modified.
- Standardize how embedded rate limits, headers, servers, url matches,
  rewrites, and sub-settings get handled on API backends. While removing
  these items generally worked as expected (due to differences in how
  these nested association attributes got assigned), this tweaks a few
  things so handling is more uniform and better tested.
- Ensure that if an embedded field isn't present in the JSON update for
  an API update, that the API remain the same (rather than removing the
  field from the record). The field will only be removed from the record
  if the field is present in the JSON, but explicitly null or an empty
  array. This better aligns with how all the other attributes are
  handled if the field is missing.
    - This does have a potential change in behavior if using
      x-www-form-urlencoded POSTs, since x-www-form-urlencoded data
      doesn't really have a strong concept of null versus not being
      present. We explicitly handle the one case of the users API, since
      the signup form calls this using x-www-form-urlencoded data.
      However, the signup form doesn't actually deal with any of these
      embedded associations, so it's probably moot.
    - Better align all the tests to use JSON for the POST bodies,
      instead of x-www-form-urlencoded, since the admin almost
      exclusively uses JSON. For the few places where
      x-www-form-urlencoded are used, we'll keep the tests as-is, but we
      should probably make an effort to get everything migrated to JSON,
      so we don't have to support some of the ambiguities
      x-www-form-urlencoded brings.
@GUI GUI added this to the v0.14.2 milestone May 26, 2017
@GUI
Copy link
Member

GUI commented May 26, 2017

This should now be fixed in v0.14.2.

It turned out this became broken in the v0.14.0 release, and this also impacted a few other "array" types in the admin (eg, you couldn't remove the last group assignment from an admin account). So thanks again for finding this! We have a lot more test coverage now to ensure this doesn't crop up again.

The 193082b commit message contains more details:

This fixes various issues surrounding embedded array handling and not being able to remove the last item. This was a regression in API Umbrella v0.14.0, due to the Rails 4 upgrade and differences in how Rails permitted params ignores array fields set to an empty array or null by default.

This came up in #367 due to not being able to remove the last required role from an API backend. However, this also impacted other areas of the admin and led to some better standardization, cleanup, and testing for other embedded array handling.

  • Fix removing the last role from an API backend not working.
  • Fix removing the last role from a user actually resulting in a role name of "" being assigned.
  • Fix not being able to remove the last group from an admin user.
  • Fix a bug with API backend throwing a Mongo error during saves if a sub-setting record was removed at the same time other sub-setting records were modified.
  • Standardize how embedded rate limits, headers, servers, url matches, rewrites, and sub-settings get handled on API backends. While removing these items generally worked as expected (due to differences in how these nested association attributes got assigned), this tweaks a few things so handling is more uniform and better tested.
  • Ensure that if an embedded field isn't present in the JSON update for an API update, that the API remain the same (rather than removing the field from the record). The field will only be removed from the record if the field is present in the JSON, but explicitly null or an empty array. This better aligns with how all the other attributes are handled if the field is missing.
    • This does have a potential change in behavior if using x-www-form-urlencoded POSTs, since x-www-form-urlencoded data doesn't really have a strong concept of null versus not being present. We explicitly handle the one case of the users API, since the signup form calls this using x-www-form-urlencoded data. However, the signup form doesn't actually deal with any of these embedded associations, so it's probably moot.
    • Better align all the tests to use JSON for the POST bodies, instead of x-www-form-urlencoded, since the admin almost exclusively uses JSON. For the few places where x-www-form-urlencoded are used, we'll keep the tests as-is, but we should probably make an effort to get everything migrated to JSON, so we don't have to support some of the ambiguities x-www-form-urlencoded brings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants