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

Jump to bottom

"url" field is undefined when restricting returned fields via the API #7089

Merged
merged 1 commit into from Jul 18, 2016
Merged

"url" field is undefined when restricting returned fields via the API #7089

merged 1 commit into from Jul 18, 2016

Conversation

vkandy
Copy link
Contributor

@vkandy vkandy commented Jul 14, 2016

Closes #6625

GET /posts query with ?field=url as query string returns "undefined" as url value. The url property is constructed from id, published_dt, and slug. Post instances are loaded with attributes given in fields parameter. If the fields parameter doesn't contain id, published_dt, and slug, URL can't be constructed and undefined is returned.

Fix was to load the 3 required properties when field parameter contains url. Response is sent after filtering out any properties that were not requested.

  • "url" and "author" fields depend on {id, published_at, slug, author_id} to construct post url.
  • results are transformed by filtering out additional properties to return just the requested fields

@vkandy vkandy changed the title Post url is constructed by loading properties additional properties [WIP] "url" field is undefined when restricting returned fields via the API Jul 14, 2016
@vkandy
Copy link
Contributor Author

vkandy commented Jul 14, 2016

Please review.

Response returns author_id when ?field=author is passed. Do we return name or id or both? The docs aren't clear: http://api.ghost.org/docs/fields

@ErisDS
Copy link
Member

ErisDS commented Jul 14, 2016

The author/author_id aspect is a slightly blurry boundary because it is a related object.

Posts have a field called author_id and a related object called author. Usually control over this is provided by using ?include=author to include the full related object instead of the id.

Ideally at this point, you would be able to say include=author&fields=author.name in order to get just an author name, but handling joins in fields is a separate issue.

@kirrg001 kirrg001 mentioned this pull request Jul 14, 2016
Closed
@ErisDS
Copy link
Member

ErisDS commented Jul 14, 2016

I have tested this out with the reproduction steps listed in this comment and am no longer able to reproduce either bug 😉

@vkandy vkandy changed the title [WIP] "url" field is undefined when restricting returned fields via the API "url" field is undefined when restricting returned fields via the API Jul 14, 2016
@vkandy
Copy link
Contributor Author

vkandy commented Jul 15, 2016

Added a test case for this fix. Good for review.

@kirrg001
Copy link
Contributor

i was able to reproduce with postman. will review

kirrg001
kirrg001 reviewed Jul 15, 2016
View reviewed changes
core/server/api/posts.js
if (_.isArray(options.columns)) {
requestedColumns = _.clone(options.columns);
if (_.includes(options.columns, 'url') || _.includes(options.columns, 'author')) {
options.columns.push('id', 'published_at', 'slug', 'author_id');

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@vkandy
Copy link
Contributor Author

vkandy commented Jul 18, 2016

Based on @kirrg001 suggestion above, implemented a more generic solution. Base class for models defines defaultColumnsToFetch() which is overridden by subclasses. In this case, Posts overrides this method and returns ['id', 'published_at', 'slug', 'author_id'].

findPage() loads model instances by performing union of permitted columns and default columns declared by models.

Finally, the response from toJSON() is filtered and only columns requested by the client are returned.

@kirrg001 kirrg001 self-assigned this Jul 18, 2016
kirrg001
kirrg001 reviewed Jul 18, 2016
View reviewed changes
core/server/models/base/index.js
@@ -355,14 +362,21 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
}

return itemCollection.fetchPage(options).then(function formatResponse(response) {
var data = {};
options.columns = allColumns;

This comment was marked as abuse.

Sign in to view

@ErisDS
Copy link
Member

ErisDS commented Jul 18, 2016

The failing lint build was a random travis error - this is green now!

@vkandy
Post url is constructed by loading properties additional properties
56c4928 
- "url" and "author" fields depend on {id, published_at, slug, author_id} to construct post url.
- implemented a generic solution by defining defaultColumnsToFetch() in
  base class for models.
- findPage() calls defaultColumnsToFetch() before loading models
- results are transformed by filtering out additional properties to return just the requested fields
- Added a test case to check for url and author fields
- Renamed allColumns as requestedColumns and used _.map instead of Promise.map
@vkandy
Copy link
Contributor Author

vkandy commented Jul 18, 2016

As per the suggestions above, renamed allColumns to requestedColumns (these are the attributes client asked for).
Replaced Promise.map() with _.map(). My rationale was that we could benefit from concurrency on large arrays but I guess the savings are minimal and _.map is more readable.

@kirrg001 kirrg001 merged commit ffd3ec5 into TryGhost:master Jul 18, 2016
chris-brown pushed a commit to chris-brown/Ghost that referenced this pull request Aug 14, 2016
@vkandy @chris-brown
fix: "url" field is undefined when restricting returned fields (TryGh…
27a1868 
…ost#7089)

closes TryGhost#6625

- "url" and "author" fields depend on {id, published_at, slug, author_id} to construct post url.
- implemented a generic solution by defining defaultColumnsToFetch() in
  base class for models.
- findPage() calls defaultColumnsToFetch() before loading models
- results are transformed by filtering out additional properties to return just the requested fields
- Added a test case to check for url and author fields
- Renamed allColumns as requestedColumns and used _.map instead of Promise.map
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this pull request Nov 20, 2016
@vkandy @geekhuyang
fix: "url" field is undefined when restricting returned fields (TryGh…
b55692f 
…ost#7089)

closes TryGhost#6625

- "url" and "author" fields depend on {id, published_at, slug, author_id} to construct post url.
- implemented a generic solution by defining defaultColumnsToFetch() in
  base class for models.
- findPage() calls defaultColumnsToFetch() before loading models
- results are transformed by filtering out additional properties to return just the requested fields
- Added a test case to check for url and author fields
- Renamed allColumns as requestedColumns and used _.map instead of Promise.map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kirrg001 kirrg001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vkandy @ErisDS @kirrg001