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

Generate schema location urls in controller #71

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Conversation

s-andringa
Copy link

Makes schema location URLs be generated in the controller. This way required keyword arguments for the URL helper are automatically revolved from the current path parameters. This solves an issues when Scimitar's routes are mounted inside a namespace with variable parameters. E.g. in our multitenant environment we have:

namespace :scim do
  namespace :v2 do
    resources :organizations, only: [] do
      mount Scimitar::Engine, at: '/'
      # etc...
    end
  end
end

In this case #scim_schemas_url requires the :organization_id key, which is omitted when the URL is generated in Schema::Base#as_json. This causes an error. Using the controller's URL helper fixes this.

Additionally, the controller's URL helpers can be overridden in a simple(r) manner, making Scimitar a bit more extensible. I have utilized this to test this change (see the application controller mixin for the tests).

The resource types controller already does a similar thing for resource type urls.

@@ -13,7 +13,7 @@ def initialize(options = {})

# Converts the schema to its json representation that will be returned by /SCHEMAS end-point of a SCIM service provider.
def as_json(options = {})
@meta.location = Scimitar::Engine.routes.url_helpers.scim_schemas_path(name: id)
@meta.location ||= Scimitar::Engine.routes.url_helpers.scim_schemas_path(name: id)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if generating a url here still has any value. I left it in here for backwards compatibility.

@pond
Copy link
Member

pond commented Oct 30, 2023

Took a while to get my head around it and I do worry about that there's now no test coverage for any prior initialisers where the likes of #scim_schemas_url is not overridden, but that'd be tricky to figure out and perhaps not worth the effort.

The only thing holding this back from merging is an addition to README.md to let people know that this feature even exists. It looks like it'd fit in quite nicely around line 92 in the Routes section. You can see some examples of engine configuration documentation above, so a few lines to say that the routing can be overridden in a demonstrated way would be great.

Is that OK?

@s-andringa
Copy link
Author

Is that OK?

No problem. See last commit.

Thanks for taking the time to consider and review my changes, and for maintaining this gem in general. Saved my life!

@pond pond merged commit 484754c into RIPAGlobal:main Oct 31, 2023
4 checks passed
@pond
Copy link
Member

pond commented Oct 31, 2023

Great stuff. Merged! Many thanks for the contribution 👍

@pond
Copy link
Member

pond commented Nov 15, 2023

This is now available via RubyGems in v2.6.0 / v1.7.0.

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

Successfully merging this pull request may close these issues.

None yet

2 participants