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

Scripts API enhancements #259

Closed
atruskie opened this issue Feb 24, 2016 · 12 comments
Closed

Scripts API enhancements #259

atruskie opened this issue Feb 24, 2016 · 12 comments

Comments

@atruskie
Copy link
Member

Please expose the following in the scripts API:

  • The #show endpoint
  • The executable_settings field and (if Add script settings mime type field #258 is a go) the executable_settings_filename field
  • and discuss: should /filter return all versions of a script? It currently does.
    • personal preference is to not allow this... but the corollary is how do we allow seeing older versions?
@cofiem
Copy link
Contributor

cofiem commented Feb 26, 2016

Is there any issue with /scripts/filter returning all versions? Restricting to most recent would be a custom change just for that model. What's the benefit? You can always find most recent version by sorting by version. Perhaps exposing group_id would make it easier to find the most recent version for each script group?

@atruskie
Copy link
Member Author

Oh dang, I thought analysis_identifier === group_id. Remind me, what is analysis_identifier used for?

Yes group_id needs to be exposed. Use case at the moment is users creating custom jobs. Assumption is they will only want latest version.

  • options:
    • custom modification of model
    • download full list every time (payloads can be quite large given config files)
    • or implement group by and max in filter API (sorting won't help)

can you think of any other methods? thoughts?

@cofiem
Copy link
Contributor

cofiem commented Feb 26, 2016

analysis_identifier is the tag used for any annotations created by an analysis.

@cofiem cofiem closed this as completed in 320c722 Feb 26, 2016
@cofiem
Copy link
Contributor

cofiem commented Feb 26, 2016

Solution I used was to only restrict scripts#index to most recent versions of each script group. scripts#filter will still return every script.

Reopen and comment if this doesn't satisfy your use cases.

@atruskie
Copy link
Member Author

Seems reasonable.

I'm looking at that diff - it doesn't seem to be covered by a test case (and thus documentation either). Am I missing something?

@cofiem
Copy link
Contributor

cofiem commented Feb 27, 2016

No, there's no test. I figured this was more an exploratory one - if you can try it out, see if it works, I'll add tests once you're satisfied.

@atruskie
Copy link
Member Author

Reopening until we've settled on API

@atruskie atruskie reopened this Feb 27, 2016
@cofiem
Copy link
Contributor

cofiem commented Mar 26, 2016

What's the status on this one?

@atruskie
Copy link
Member Author

I've been using dummy data but that doesn't really matter. On reflection I'm a little uncomfortable with the non-standard (and undocumented) behaviour.

Had another thought. This problem occurs because we need a group by operation. Perhaps this means this resource is sufficiently complicated enough to need a nested resource. I.e. /scripts should behave as every other endpoint does and /scripts/latest represents the latest versions - with only all the standard read operations (index, show, filter). This has the advantage that our docs would clearly document this behaviour...

This would set a precedence for wider API design - we have other similar problems.

Sorry to be a pain 😬

@cofiem
Copy link
Contributor

cofiem commented Mar 26, 2016

I don't understand what you want me to do.

Do you want me to do the nested /scripts/latest? Or do you want me to look into group by? I'd really rather not do group by - I purposefully designed the filter api to not allow it. It would probably require some re-working of the api. Nested routes are a safer bet from my point of view.

Thoughts?

@atruskie
Copy link
Member Author

I need to think about other areas where this kind of API problem will exist. I'll get back to you.

@atruskie
Copy link
Member Author

Okay, I've thought about this. I thought there were similar patterns in our database structure but I couldn't identify them.

I've also consulted various guides on designing RESTFUL interfaces. My proposed solution has been put in the API wiki doc (see changeset).

In short, the TODO list:

  • no similar pattern exists at this point - don't over engineer the following solution
  • expose /scripts normally
  • expose new nested resource named /scripts/groups with three actions: index, show, and filter
    • note: filter still has its extra path fragment: e.g. /scripts/groups/filter
    • note: show uses the group_id to select a group of scripts
  • add the following calculated properties to the standard scripts model (used for both /scripts and /scripts/groups:
    • is_latest
    • is_original
  • group the output results via a simple custom method

My naive suggestion is to do all the processing normally and group the results in the last step of the controller actions.

This model allows for all use-cases I can think of, is extensible (we can add more properties to the model... unlike the /scripts/latest idea which has a fixed, non-generalised, non-reusable, representation), and it is self documenting because the idea of groups is clearly expressed in the path. Filtering for latest scripts can be done with standard /filter spec - we just filter for is_latest eq true.

Provided this is easy enough and you agree with me, LGTM.

@atruskie atruskie self-assigned this Apr 14, 2016
@atruskie atruskie added the API label Apr 19, 2016
@atruskie atruskie modified the milestone: Custom Jobs Apr 19, 2016
@cofiem cofiem closed this as completed in 4db980b Apr 26, 2016
cofiem added a commit that referenced this issue Apr 26, 2016
Implements  `is_first_version` and `is_last_version` attributes. Fixes #259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants