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

Added support for API slugs #14344

Merged
merged 4 commits into from
Mar 21, 2017
Merged

Added support for API slugs #14344

merged 4 commits into from
Mar 21, 2017

Conversation

abellotti
Copy link
Member

@abellotti abellotti commented Mar 15, 2017

  • added slug method on application record instances
  • returns ":collection/:id" if instance class matches a collection class or any of its descendants, nil otherwise.
  • declared as a virtual_column so it's accessible from API.
  • Added specs

Closes Issue #11635

@abellotti
Copy link
Member Author

/cc @mkanoor @gmcculloug @gtanzillo as promised.

@Fryguy please review. Thanks!!

@abellotti abellotti force-pushed the api_slugs branch 2 times, most recently from a3a5062 to dcdf739 Compare March 15, 2017 16:46
Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

A few comments, but overall this looks good. Maybe add a negative test for a new record that does not have an ID value yet for a valid class.

# API Support
virtual_column :slug, :type => :string

def slug
Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy Should this logic be moved into a separate file and included here like the Ar* modules above.

Is slug a descriptive enough method name. Is url_slug or href_slug more useful? Just a thought.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer url_slug, myself.

virtual_column :slug, :type => :string

def slug
collection = Api::CollectionConfig.new.name_for_subklass(self.class)
Copy link
Member

Choose a reason for hiding this comment

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

Suggest returning early if id is nil so you do not spend time in name_for_subklass for new records that do not have an ID yet.

def slug
  return nil unless id
  collection = Api::CollectionConfig.new.name_for_subklass(self.class)
  "#{collection}/#{id}" if collection
end

@gmcculloug
Copy link
Member

cc @bzwei

@mkanoor
Copy link
Contributor

mkanoor commented Mar 17, 2017

@abellotti would it make sense to add another function like fetch_by_slug that fetches the object based on the slug that we could use internally.

A typical use case would be we have a slug for a provider that looks like providers/99 and we can fetch the ExtManagementSystems.

This code might already exist that the REST API uses to load the AR object

@abellotti
Copy link
Member Author

We can provide something like that in a separate PR. API internally does not use slugs, so we'd write something similar to resource_search but simply takes the slug.

@abellotti
Copy link
Member Author

/cc @gmcculloug @gtanzillo I went with href_slug since it pertains to the unique identifier of the href presented by the API. Thanks.

@gmcculloug
Copy link
Member

Thanks, looks good. @Fryguy Any thoughts here?

@@ -73,6 +73,13 @@ def name_for_klass(resource_klass)
@cfg.detect { |_, spec| spec[:klass] == resource_klass.name }.try(:first)
end

def name_for_subklass(resource_klass)
Copy link
Member

@Fryguy Fryguy Mar 17, 2017

Choose a reason for hiding this comment

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

I don't like subklass bit. you can just say subclass here and it won't break anything (as well as resource_class for that matter). klass is only necessary when standalone because it's a keyword.

extend ActiveSupport::Concern

included do
virtual_column :href_slug, :type => :string
Copy link
Member

@Fryguy Fryguy Mar 17, 2017

Choose a reason for hiding this comment

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

I feel like we should namespace ourselves here and call it api_href_slug and the module itself ArApiSlug or ArApiHrefSlug or something like that, because we get slugs in the providers world for other purposes and it can get confusing. e.g. ManageIQ::Providers::Redhat::Instance#href_slug. Is that the ManageIQ href slug or is that the native RHV href slug?

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting point, I wonder if the namespace should go beyond api as they have API's too. Maybe we need Miq. i.e. "miq_api_href_slug" or maybe "miq_href_slug" is sufficient in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

miq_href_slug 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

after discussing with @Fryguy, we're keeping it as href_slug as it's the most relevant/natural name w.r.t. slug for the href attribute, as compared to href and miq_href_slug.

In a separate PR though (replication), we should check/enforce that no href or href_slug attributes are named as such as it would clash with the API.

Copy link
Member

Choose a reason for hiding this comment

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

@abellotti @Fryguy Did you also discuss the module name which is mentioned in this chain? Are you keeping ArSlug or should that be changing in this PR?

Copy link
Member Author

@abellotti abellotti Mar 21, 2017

Choose a reason for hiding this comment

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

haven't discussed that. I'm game to changing it if that's desired, is there a preference for ArHrefSlug maybe ? @Fryguy ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think make it match the method, so ArHrefSlug

def href_slug
return unless id
collection = Api::CollectionConfig.new.name_for_subklass(self.class)
"#{collection}/#{id}" if collection
Copy link
Member

@Fryguy Fryguy Mar 17, 2017

Choose a reason for hiding this comment

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

I feel like this should be a builder method in Api namespace, that we just call here. Thinking something like...

# here

def href_slug
  Api.build_href_slug(self.class, id)
end

# there  Not sure the best module, but that's up to you

module Api
  def self.build_href_slug(klass, id)
    return unless id
    collection = Api::CollectionConfig.new.name_for_subklass(klass)
    "#{collection}/#{id}" if collection
  end
end

This way the knowledge for building slugs generically belongs with the API and the API specs, irrespective of having an AR instance (i.e. it can also be called (and tested) with only a type/id pair and not an instance). And the knowledge of how to build it for AR instance then is just a simple call with some defaults from the instance.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, then name_for_subklass actually just becomes a private method

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this assume that all objects that are configured in the API have a primary collection?

- added slug method on application record instances
- returns ":collection/:id" if instance class matches
  a collection class or any of its descendants, nil otherwise.
- declared as a virtual_column so it's accessible from API.
- Added specs

Closes Issue ManageIQ#11635
@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commits abellotti/manageiq@2f3c950~...f0de33b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks good. 🍰

@Fryguy Fryguy merged commit 5059d08 into ManageIQ:master Mar 21, 2017
@Fryguy Fryguy added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 21, 2017
@abellotti abellotti deleted the api_slugs branch April 11, 2017 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants