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

/v1/module handles fields parameter in a way that produces single-value arrays #580

Closed
tsibley opened this issue Nov 22, 2016 · 6 comments
Closed
Assignees

Comments

@tsibley
Copy link
Contributor

tsibley commented Nov 22, 2016

Compare:

$ curl -fsS https://api.metacpan.org/v0/module/Pod::Cpandoc?fields=distribution
{
   "distribution" : "Pod-Cpandoc"
}

$ curl -fsS https://fastapi.metacpan.org/v1/module/Pod::Cpandoc?fields=distribution
{
   "distribution" : [
      "Pod-Cpandoc"
   ]
}

$ curl -fsS https://fastapi.metacpan.org/v1/module/Pod::Cpandoc
{
   …,
   "distribution" : "Pod-Cpandoc",
   …
}

I'll take a look at this in the next couple days if no one else does.

@tsibley tsibley self-assigned this Nov 22, 2016
@ranguard
Copy link
Member

I know the 'utility' of flattening arrays is nice, but if it leads to our code having lots of these we should seriously consider just keeping them as arrays all the time

@ranguard
Copy link
Member

s/keeping them/keeping results/

@tsibley
Copy link
Contributor Author

tsibley commented Nov 22, 2016

Well, it's a huge PITA for consumers which otherwise would just be able to change endpoints and be done with it.

@tsibley
Copy link
Contributor Author

tsibley commented Nov 22, 2016

For this particular endpoint, we obviously take pains to collapse single-valued arrays, but miss the case when the fields parameter is specified or something. (I haven't looked at the code yet.)

@tsibley
Copy link
Contributor Author

tsibley commented Nov 22, 2016

We can also refactor the controller/model code such that there's only a few places where the output gets filtered through our munger.

tsibley added a commit that referenced this issue Nov 23, 2016
…ic fields

We claim in our docs¹ that our convenience endpoints revert the newer
Elasticsearch behaviour of wrapping single values in arrays.  This makes
that so in more places, specifically the places where the client has
requested a limited subset of fields.

While this may seem like a pain for us and lead to uglier code, think
about it this way: either we do it for our API one time, or every client
that runs into the issue has to do it themselves many times over.

A better solution may exist which centralizes application of
single_valued_arrayref_to_scalar(), but this works for now.

See also GH#580 for an example case.²

¹ https://github.com/metacpan/metacpan-examples/blob/master/README.md#upgrading-from-v0
² #580
tsibley added a commit that referenced this issue Nov 26, 2016
…ic fields

We claim in our docs¹ that our convenience endpoints revert the newer
Elasticsearch behaviour of wrapping single values in arrays.  This makes
that so in more places, specifically the places where the client has
requested a limited subset of fields.

While this may seem like a pain for us and lead to uglier code, think
about it this way: either we do it for our API one time, or every client
that runs into the issue has to do it themselves many times over.

A better solution may exist which centralizes application of
single_valued_arrayref_to_scalar(), but this works for now.

See also GH#580 for an example case.²

This reverts a small change to tests which was made to accommodate the
new ES behaviour.

¹ https://github.com/metacpan/metacpan-examples/blob/master/README.md#upgrading-from-v0
² #580
@tsibley
Copy link
Contributor Author

tsibley commented Nov 29, 2016

Fixed in #581.

@tsibley tsibley closed this as completed Nov 29, 2016
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

No branches or pull requests

2 participants