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

Add ApiVersion coercion_modes and fetch_known_versions from Shopify #600

Merged
merged 6 commits into from
Sep 3, 2019

Conversation

jtgrenz
Copy link
Contributor

@jtgrenz jtgrenz commented Aug 8, 2019

This PR allows us to dynamically define api_versions based on the response from https://app.shopify.com/services/apis.json

This removes the need to manually updated the gem every time a new version is released or set to end of life.

When the gem is required, we define a set of known versions from the unauthenticated services/apis.json endpoint and everything behaves more or less like it did before. If an unknown version is attempted to be set (ie as of today, 2020-04 or taco-42) an UnknownVersion error is raised with all known versions as message.

If for some reason the known_version_set cannot be initialized at start, then we default to assuming the version is valid, and rely on the server to handle the request and return an error if necessary. Developers can retry defining the version set by calling ShopifyAPI::ApiVersion.define_known_versions at anytime. Any version that was fetched from the server will have persisted? return true, and any on the fly version instances would return false.

ApiVersion attributes were solidified internally at shopify I think after version support was added to the gem, so I also deprecated and aliased methods like #name and #stable? to #handle and #supported?

@jtgrenz jtgrenz requested a review from a team as a code owner August 8, 2019 20:36
lib/shopify_api/api_version.rb Outdated Show resolved Hide resolved
lib/shopify_api/api_version.rb Outdated Show resolved Hide resolved
lib/shopify_api/meta.rb Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tanema tanema left a comment

Choose a reason for hiding this comment

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

I have a couple questions but no show stoppers

README.md Outdated Show resolved Hide resolved
lib/shopify_api/meta.rb Show resolved Hide resolved
lib/shopify_api/api_version.rb Outdated Show resolved Hide resolved
@versions = {}
def latest_stable_version
warn(
'[DEPRECATED] ShopifyAPI::ApiVersion.latest_stable_version is deprecated and will be removed in a future version.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a recommended way to do this same thing after this method is removed? Is this just discouraged going forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we wanted to discourage anyone trying to set the version to the latest version automatically since it's likely to break apps. Bumping to the latest version should hopefully be intentional.

It can still be computed however since each ApiVersion has a latest_supported attribute, its just a little more work.

ShopifyAPI::Meta.admin_versions.find(&:latest_supported?) works at the expense of another api call, but we could also cache the versions in Meta.admin_versions

Copy link
Contributor

Choose a reason for hiding this comment

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

latest_supported_version is used by the ShopifyApp gem https://github.com/Shopify/shopify_app/blob/0900ddcb2f268f6efe3e184739ee83eeab3a0020/lib/generators/shopify_app/install/install_generator.rb#L12 to run the generators.

It can now use ShopifyAPI::Meta.admin_versions.find(&:latest_supported?) to find that info.

@jtgrenz
Copy link
Contributor Author

jtgrenz commented Aug 12, 2019

updated per @alexaitken 's comments on slack. ApiVersion now supports two coercion modes. The default will just accept whatever handle you give it and leave it to the server to deal with any invalid versions, probably what you would want in production since you should have everything tested already.

The second stricter mode will not allow you to set any version that is not pre-defined. Setting ApiVersion.coercion_mode = :predefined_only will require you to fetch the known versions from Shopify with ApiVersion.define_known_versions. Theres no more remote request when requiring the gem and any developers who want to pre-define versions on boot can do so in an initializer.

Little more flexibility for different developers for different environments.

Readme still needs to be updated and this needs a rebase

@jtgrenz
Copy link
Contributor Author

jtgrenz commented Aug 15, 2019

Just pushed an update and rebased against master.

I made ShopifyAPI::ApiVersion a PORO so its more light weight like Alex suggested. This now means that ShopifyAPI::Meta.admin_versions no longer returns an array of ShopifyAPI::ApiVersions but instead falls back to using ActiveResource magic and returns an array of ShopifyAPI::Meta::Versions instead.

When using ShopifyAPI::ApiVersion.define_known_versions, we still grab the array of versions from Meta but we then build and cache new lighter weight ApiVersion objects from the fetched ShopifyAPI::Meta::Version.attributes hash. Instead of using persisted? to verify if it was fetched from Shopify or just defined at runtime, you would use verified?

Copy link
Contributor

@eapache eapache 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 minor comments, and the README needs updating, but approach LGTM.


def self.define_version(version)
@versions ||= {}
def define_known_versions
Copy link
Contributor

Choose a reason for hiding this comment

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

I might call this fetch_known_versions to be explicit that it involves a remote lookup

Copy link
Contributor Author

@jtgrenz jtgrenz Aug 15, 2019

Choose a reason for hiding this comment

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

Yeah I think I had that at first and at some point changed it back to minimize the number of changes or something? 🤔 It sorta made sense at the time, but I think you're right that fetch is a better choice.

Not sure if I should just rename this or alias it and issue a depreciation warning


@versions[version.name] = version
end
def clear_defined_versions
Copy link
Contributor

Choose a reason for hiding this comment

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

clear_known_versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same concern as define/fetch_known_versions

lib/shopify_api/meta.rb Show resolved Hide resolved
lib/shopify_api/api_version.rb Outdated Show resolved Hide resolved
@jtgrenz jtgrenz changed the title dynamically define api version from server at boot Add ApiVersion coercion_modes and fetch_known_versions from Shopify Aug 15, 2019
lib/shopify_api/meta.rb Outdated Show resolved Hide resolved
@jtgrenz jtgrenz force-pushed the auto-define-api-versions branch 2 times, most recently from 02b828a to 66e9427 Compare August 16, 2019 15:58
README.md Outdated Show resolved Hide resolved
README.md Outdated
ShopifyAPI::ApiVersion.fetch_known_versions
```

Known versions are fetched and cached from https://app.shopify.com/services/apis.json. Trying to use a version outside this set will raise an error. To switch back to naïve lookup and create a version if its not found, call `ShopifyAPI::ApiVersion.version_lookup_mode = :define_on_unknown` (this is the default mode).
Copy link
Contributor

Choose a reason for hiding this comment

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

We should maybe mention that this set currently-supported APIs only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it includes release candidate as well which is unsupported.

@jtgrenz
Copy link
Contributor Author

jtgrenz commented Aug 16, 2019

Just pushed an update that cleans up a few things. I suppose this is a good time to summarize the whole change again.

New

  • Adds ShopifyAPI::Meta. This Meta class inherits from ActiveResource::Base and not ShopifyAPI::Base. It returns read only information about our Apis and their versions. It fetches data from https://app.shopify.com/services/apis.json and does not use authentication.
  • Adds apis.json and api_versions.json test fixtures.
  • Adds MetaTestCase to test that Meta.admin_versions returns an array of admin versions.

Changed

ShopifyAPI::ApiVersion

Class methods

  • replaces self. class methods with class << self style methods.
  • adds .versions method to return hash of known versions in { handle: version.attributes } format.
  • adds .version_lookup_mode and .version_lookup_mode= which changes how we look up versions from the know versions list. Valid options are :raise_on_unknown and :define_on_unknown
  • adds .find_version which returns a version from the known versions set or instantiates a new one and adds it to the set (based on version_lookup_mode)
  • adds .fetch_known_versions which looks up version info from server via ShopifyAPI::Meta and adds them to the known version set.
  • adds .add_to_known_versions which adds a version to the known version set
  • adds private .sanitize_known_versions which removes any version from the known set that is not verified.
  • adds private .unknown_version_error_message which returns an error message for an unknown version when version_lookup_mode is :raise_on_unknown.
  • Adds ApiVersion.initialize. Accepts an attributes hash argument. handle: is required, all other attributes default to false.

Instance Methods

  • redefines #construct_api_path and #construct_graphql_path to use the version handle in the path.
  • adds #handle_as_date method to convert a version handle into a date for comparison. If handle == :unstable, the date is set to the year 3000 so its always in the future.
  • adds #unstable? which is true if handle == 'unstable'
  • adds accessors for all attributes.
  • adds #== equality method. Returns true if the version handles between two objects match.

Misc

  • deletes ShopifyAPI::DefinedVersions
  • removes ShopifyAPI::ApiVersion.define_known_versions from shopify_api.rb
  • updates calls to deprecated methods to use new methods.
  • updates readme.

Deprecated

ApiVersion

  • .coerce_to_version deprecated. use .find_version
  • .define_known_versions deprecated. Use .fetch_known_versions
  • .clear_defined_versions deprecated. Use. .clear_known_versions
  • .latest_stable_version deprecated. Use ShopifyAPI::Meta.admin_versions.find(&:latest_supported)
  • #name deprecated. Use #handle
  • #stable? deprecated. Use #supported?.

Removed

  • Removed ShopifyAPI::ApiVersion::Release and ShopifyAPI::ApiVersion::Unstable. Theres nothing really special about these that require another class.

Copy link
Contributor

@tylerball tylerball left a comment

Choose a reason for hiding this comment

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

This approach is great, after living with API Versioning for a while I think it's clear we should leave it up to developers to define which versions they're using.

Related to this, I'm not sure developers will use predefined_only, as its a more involved setup and extra request when they could just set and forget. I'm okay with it being available though, as it could be useful in our tooling and shopify_apps generators as discussed.

Copy link
Contributor

@alexaitken alexaitken left a comment

Choose a reason for hiding this comment

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

We can close #603 and go with this setup.

should this be a breaking change that requires a major version bump @nwtn @tylerball

@jtgrenz
Copy link
Contributor Author

jtgrenz commented Aug 29, 2019

sweet. I'll fix these merge conflicts and 🚢

@jtgrenz jtgrenz force-pushed the auto-define-api-versions branch 2 times, most recently from aa71979 to 1b9b8de Compare August 29, 2019 16:13
@jtgrenz
Copy link
Contributor Author

jtgrenz commented Aug 29, 2019

fixed merge conflicts and rebased after merging #605. I added a new commit to add more methods to ApiVersion::NullVersion so it raises a more useful error if you try to call any of the new methods added to ApiVersion (like handle, display_name, verified?, etc).

@jtgrenz
Copy link
Contributor Author

jtgrenz commented Sep 3, 2019

Forgot about this on friday. Rebased again and it should be good to merge as soon as CI is done.

@tylerball did we want to release this as version 8.0.0? Is there anything else we need to include in a major version bump?

@jtgrenz jtgrenz merged commit 7c5a458 into master Sep 3, 2019
@jtgrenz jtgrenz deleted the auto-define-api-versions branch September 3, 2019 14:39
@jtgrenz jtgrenz temporarily deployed to rubygems September 5, 2019 14:13 Inactive
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

5 participants