API Enhancement to support fine-grained settings whitelisting #13948

abellotti · 2017-02-16T18:31:18Z

Adding support for arbitrary path categories, can include top level categories like it does today like product, authentication, server, but also sub pathing, server/role, session/timeout even lower level entries like server/worker_monitor/start_algorithm/value
Exposes the subset of the settings as driven by the new categories
Can fetch top level categories or any setting entry path:

GET /api/settings/product, or GET /api/settings/server/role (if exposed)
Added rspecs

abellotti · 2017-02-16T18:31:52Z

Marked as WIP

prototype
requires tests

abellotti · 2017-02-16T18:36:07Z

This is a prototype for the enhancement I briefly referred to. It allows the API to expose a subset of the settings tree, down to an individual entry, and allows the users to be able to fetch any particular entry and not just top level category (i.e. GET /api/settings/product) with is a current limitation.

For David's use, we can update the config/api.yml to expose the additional entries he needs as well as others as follows:

:collections:
  :settings:
    :categories:
      - product
      - server/role
      - server/timezone
      - server/asynchronous_notifications

And with this PR: a GET of the settings collection:

GET /api/settings

will return the exposed settings subtree as follows:

{ 
  "product": {
    "maindb": "ExtManagementSystem",
    "container_deployment_wizard": false,
    "datawarehouse_manager": false
  },
  "server": {
    "role" : "database_operations,event,reporting,scheduler,smartstate,ems_operations,ems_inventory,user_interface,websocket,web_services,automate"
    "timezone" : "UTC",
    "asynchronous_notifications" : false
  }
}

One can fetch just the server subtree:

GET /api/settings/server

{
  "server": {
    "role" : "database_operations,event,reporting,scheduler,smartstate,ems_operations,ems_inventory,user_interface,websocket,web_services,automate"
    "timezone" : "UTC",
    "asynchronous_notifications" : false
  }
}

Or simply the entry of interest:

GET /api/settings/server/asynchronous_notification

{
  "server": {
    "asynchronous_notifications" : false
  }
}

abellotti · 2017-03-08T21:51:47Z

@imtayadeway @gtanzillo please review. Thanks!!

imtayadeway · 2017-03-09T16:39:02Z

spec/requests/api/settings_spec.rb

+      run_get settings_url
+
+      expect(response.parsed_body).to match(
+        "product"        => normalize_settings(Settings.product),


I think if we stubbed the Settings constant as below, we'd have more control over this. Then you could see in these tests that you're getting back the values you expected. This test gives me low confidence, because it's verifying against something that's not specified (Settings.product etc.., whatever these values may be), and it's also munging those values with untested code (normalize_hash).

correct, if we have a declared Settings hash (without symbol or nil data), the normalize_hash wouldn't be needed. It was written as to make sure the Settings returned is how the API would have rendered it.

imtayadeway · 2017-03-09T16:43:00Z

app/controllers/api/settings_controller.rb

    end

    private

-    def exposed_settings
-      ApiConfig.collections[:settings][:categories]
+    def whitelist_settings(settings)


This seems (a) sufficiently complicated, and (b) already abstracted away from details of the request to warrant a service object that could be perhaps better tested in isolation. WDYT?

This method is coded as it's needed as such by a following PR. Some of this code will be moving to an API Settings mixin, will be used by servers, regions and zones.

Not sure I follow. Is there a problem with delegating?

imtayadeway · 2017-03-09T18:52:19Z

Not sure I get the workflow around configurable categories. Is it designed to be configurable by the end user? How would they configure it, by updating the yml file by hand?

Fryguy · 2017-03-10T21:22:48Z

app/controllers/api/settings_controller.rb

+    def settings_entry_to_hash(path, value)
+      path_elements = path.split("/")
+      result_hash = {path_elements.last => value}
+      path_elements[0..-2].reverse.each { |entry| result_hash = { entry => result_hash } } if path_elements.length > 1


This might be cleaner if you just start with the value and iterate from there.

def settings_entry_to_hash(path, value) result = value path.split("/").reverse.each { |entry| result = {entry => result} } result end

Actually, now that I think about it, just use store_path

def settings_entry_to_hash(path, value) {}.tap { |h| h.store_path(path.split("/"), value) } end

Example

path = "foo/bar/baz" value = 1234 {}.tap { |h| h.store_path(path.split("/"), value) } # => {"foo"=>{"bar"=>{"baz"=>1234}}}

Additionally, if you're just using store_path, then you can avoid the create-entry-and-deep-merge-loop I think.

Ahh, didn't know about store_path, NICE !! I'll update.

deep_merge was still needed with categories like server/role, server/worker_monitor, ...

abellotti · 2017-03-10T22:33:20Z

@imtayadeway as discussed yesterday, see updated tests, much simpler. Thanks!!

categories like it does today like product, authentication, server, but also sub pathing, server/role, session/timeout even lower level entries like server/worker_monitor/start_algorithm/value - whitelists a subset of the settings as driven by the new categories - Can fetch top level categories or any setting entry path: GET /api/settings/product, or GET /api/settings/server/role (if exposed) - Added rspecs

and thus queried by the API eliminating the need for Settings normalization on the responses in the tests.

settings categories via the method stub_api_settings_categories. - Moved the method to within the settings_spec.rb's tests

miq-bot · 2017-03-15T21:53:06Z

Checked commits abellotti/manageiq@7df0015~...86cd8ff with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

abellotti added api enhancement wip labels Feb 16, 2017

abellotti assigned gtanzillo Feb 16, 2017

abellotti force-pushed the api_fine_grained_settings branch 2 times, most recently from 41ca011 to c22ac82 Compare March 8, 2017 21:47

abellotti changed the title ~~[WIP] API Enhancement to support fine-grained settings exposure~~ API Enhancement to support fine-grained settings whitelisting Mar 8, 2017

abellotti removed the wip label Mar 8, 2017

abellotti force-pushed the api_fine_grained_settings branch from c22ac82 to 0d5a8b8 Compare March 9, 2017 02:18

imtayadeway reviewed Mar 9, 2017

View reviewed changes

Fryguy reviewed Mar 10, 2017

View reviewed changes

abellotti force-pushed the api_fine_grained_settings branch from 0d5a8b8 to ebdce57 Compare March 10, 2017 21:39

abellotti added 4 commits March 15, 2017 17:43

Updated rspecs so a well defined set of the settings gets merged

0354870

and thus queried by the API eliminating the need for Settings normalization on the responses in the tests.

Simplified logic of settings_entry_to_hash by leveraging store_path

69203c8

- Reduced the scope of the stub_api_collection_config to just the

86cd8ff

settings categories via the method stub_api_settings_categories. - Moved the method to within the settings_spec.rb's tests

abellotti force-pushed the api_fine_grained_settings branch from d2912c5 to 86cd8ff Compare March 15, 2017 21:50

imtayadeway approved these changes Mar 15, 2017

View reviewed changes

gtanzillo added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 16, 2017

gtanzillo merged commit 7af6fcd into ManageIQ:master Mar 16, 2017

abellotti deleted the api_fine_grained_settings branch March 17, 2017 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Enhancement to support fine-grained settings whitelisting #13948

API Enhancement to support fine-grained settings whitelisting #13948

abellotti commented Feb 16, 2017 •

edited

Loading

abellotti commented Feb 16, 2017

abellotti commented Feb 16, 2017

abellotti commented Mar 8, 2017

imtayadeway Mar 9, 2017

abellotti Mar 9, 2017

imtayadeway Mar 9, 2017

abellotti Mar 9, 2017

imtayadeway Mar 9, 2017

imtayadeway commented Mar 9, 2017

Fryguy Mar 10, 2017

Fryguy Mar 10, 2017

Fryguy Mar 10, 2017

abellotti Mar 10, 2017

abellotti Mar 10, 2017

abellotti commented Mar 10, 2017

miq-bot commented Mar 15, 2017

API Enhancement to support fine-grained settings whitelisting #13948

API Enhancement to support fine-grained settings whitelisting #13948

Conversation

abellotti commented Feb 16, 2017 • edited Loading

abellotti commented Feb 16, 2017

abellotti commented Feb 16, 2017

abellotti commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Mar 10, 2017

miq-bot commented Mar 15, 2017

abellotti commented Feb 16, 2017 •

edited

Loading