Uploaded image for project: 'CouchDB'
  1. CouchDB
  2. COUCHDB-1923

Allow ?attachments=true alongside ?include_docs=true on view

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:
      None

      Description

      I have a situation where it'd be handy to get inline [base64] attachments for the documents from a view response (already using include_docs=true).

      I wonder if this is as easy a fix as COUCHDB-549 was?

        Issue Links

          Activity

          Hide
          klaus_trainer Klaus Trainer added a comment -

          <bikeshedding>Although longer, `include_attachments` would be self-explanatory and consistent with `include_docs`</bikeshedding>

          Show
          klaus_trainer Klaus Trainer added a comment - <bikeshedding>Although longer, `include_attachments` would be self-explanatory and consistent with `include_docs`</bikeshedding>
          Hide
          klaus_trainer Klaus Trainer added a comment -

          I've implemented support for a `include_attachments` query parameter for the view API, and while at it, for the changes API as well.

          https://github.com/apache/couchdb/pull/106

          Show
          klaus_trainer Klaus Trainer added a comment - I've implemented support for a `include_attachments` query parameter for the view API, and while at it, for the changes API as well. https://github.com/apache/couchdb/pull/106
          Hide
          rnewson Robert Newson added a comment -

          Needs work (specifically, all the modifications to couch_doc should come out, there's already a mechanism to fetch documents with base64 encoded attachments) and a warning in the docs about doing this for large attachments.

          Show
          rnewson Robert Newson added a comment - Needs work (specifically, all the modifications to couch_doc should come out, there's already a mechanism to fetch documents with base64 encoded attachments) and a warning in the docs about doing this for large attachments.
          Hide
          janl Jan Lehnardt added a comment -

          Robert Newson Do you have a pointer to said mechanism?

          Show
          janl Jan Lehnardt added a comment - Robert Newson Do you have a pointer to said mechanism?
          Hide
          klaus_trainer Klaus Trainer added a comment -

          Jan Lehnardt Just passing `attachments` instead of `include_attachments` to `couch_doc:to_json_obj/2` should be fine.

          I'll address Robert Newson's points and push an update soon.

          Show
          klaus_trainer Klaus Trainer added a comment - Jan Lehnardt Just passing `attachments` instead of `include_attachments` to `couch_doc:to_json_obj/2` should be fine. I'll address Robert Newson 's points and push an update soon.
          Hide
          janl Jan Lehnardt added a comment -

          Klaus Trainer ah thanks, overlooked that.

          Show
          janl Jan Lehnardt added a comment - Klaus Trainer ah thanks, overlooked that.
          Hide
          rnewson Robert Newson added a comment -

          Jan, Klaus already touched the code next to it when he added [include_attachments] as a duplicate of [attachments] rather than transform at the http level.

          Show
          rnewson Robert Newson added a comment - Jan, Klaus already touched the code next to it when he added [include_attachments] as a duplicate of [attachments] rather than transform at the http level.
          Hide
          klaus_trainer Klaus Trainer added a comment -

          Thanks for your review and good suggestions, Robert Newson. I've updated the feature branch and think that I've addressed your points.

          Show
          klaus_trainer Klaus Trainer added a comment - Thanks for your review and good suggestions, Robert Newson . I've updated the feature branch and think that I've addressed your points.
          Hide
          natevw Nathan Vander Wilt added a comment -

          So is this being implemented with plain old ?attachments to match the docs query param? The "include_attachments" suggestion was only consistent in this one context, AFAICT all other uses of the same are simply "attachments" and I'd personally prefer the latter. (Sounds like that's what is used internally too?)

          Show
          natevw Nathan Vander Wilt added a comment - So is this being implemented with plain old ?attachments to match the docs query param? The "include_attachments" suggestion was only consistent in this one context, AFAICT all other uses of the same are simply "attachments" and I'd personally prefer the latter. (Sounds like that's what is used internally too?)
          Hide
          klaus_trainer Klaus Trainer added a comment -

          > The "include_attachments" suggestion was only consistent in this one
          > context, AFAICT all other uses of the same are simply "attachments"
          > and I'd personally prefer the latter.

          Using plural suggests that there are multiple instances of `attachments`
          query parameter usage. However, the `attachments` query parameter is
          currently used for only one API endpoint: `/:db/:docid`.

          Nathan Vander Wilt: While first thinking that we can just reuse the `attachments`
          query parameter for other API endpoints as well, I came to the
          conclusion that it is better to introduce a new `include_attachments`
          query parameter for the following reasons.

          In the document API, attachments are sent with one particular
          requested document iff the query parameter value is `true`. Depending
          on whether a client requests the content type "application/json" (by
          setting the "Content-Type" header accordingly) or not, attachments are
          either included Base64-encoded in the document, or sent along as
          separate parts of the multipart response in their binary representation.

          In the view and changes API we got a collection of documents. Even if
          the `attachments` (whatsoever) query parameter value is `true`, no
          attachment contents will be sent unless the `include_docs` query
          parameter value is also `true`. That is, the effectiveness of the
          `attachments` query parameter depends on the presence and value of
          another query parameter here. Moreover, in contrast to the document
          API, attachments are always included Base64-encoded in the documents,
          and there are no multipart responses.

          Although there would be similar meaning, I don't think it would be a
          good idea to use the same query parameter name. Following the
          differences in semantics, it is consequential to use a different query
          parameter name. This makes the differences more explicit and therefore
          can help preventing users from having bad surprises.

          Show
          klaus_trainer Klaus Trainer added a comment - > The "include_attachments" suggestion was only consistent in this one > context, AFAICT all other uses of the same are simply "attachments" > and I'd personally prefer the latter. Using plural suggests that there are multiple instances of `attachments` query parameter usage. However, the `attachments` query parameter is currently used for only one API endpoint: `/:db/:docid`. Nathan Vander Wilt : While first thinking that we can just reuse the `attachments` query parameter for other API endpoints as well, I came to the conclusion that it is better to introduce a new `include_attachments` query parameter for the following reasons. In the document API, attachments are sent with one particular requested document iff the query parameter value is `true`. Depending on whether a client requests the content type "application/json" (by setting the "Content-Type" header accordingly) or not, attachments are either included Base64-encoded in the document, or sent along as separate parts of the multipart response in their binary representation. In the view and changes API we got a collection of documents. Even if the `attachments` (whatsoever) query parameter value is `true`, no attachment contents will be sent unless the `include_docs` query parameter value is also `true`. That is, the effectiveness of the `attachments` query parameter depends on the presence and value of another query parameter here. Moreover, in contrast to the document API, attachments are always included Base64-encoded in the documents, and there are no multipart responses. Although there would be similar meaning, I don't think it would be a good idea to use the same query parameter name. Following the differences in semantics, it is consequential to use a different query parameter name. This makes the differences more explicit and therefore can help preventing users from having bad surprises.
          Hide
          natevw Nathan Vander Wilt added a comment -

          > However, the `attachments` query parameter is
          > currently used for only one API endpoint: `/:db/:docid`.

          Fair point. While I don't see the encoding/multipart issues as a particularly strong argument (that's negotiated via Accept rather than params) the "no attachment contents will be sent unless the `include_docs` query parameter value is also `true`" is interesting.

          Would it make sense to share a pattern from the stale/feed parameters and turn include_docs into a ternary? E.g. `include_docs=true`, `include_docs=attachments`?

          Show
          natevw Nathan Vander Wilt added a comment - > However, the `attachments` query parameter is > currently used for only one API endpoint: `/:db/:docid`. Fair point. While I don't see the encoding/multipart issues as a particularly strong argument (that's negotiated via Accept rather than params) the "no attachment contents will be sent unless the `include_docs` query parameter value is also `true`" is interesting. Would it make sense to share a pattern from the stale/feed parameters and turn include_docs into a ternary? E.g. `include_docs=true`, `include_docs=attachments`?
          Hide
          klaus_trainer Klaus Trainer added a comment -

          > Would it make sense to share a pattern from the stale/feed parameters
          > and turn include_docs into a ternary? E.g. `include_docs=true`,
          > `include_docs=attachments`?

          Yeah, we would avoid introducing another query parameter by doing so.
          However, allowing the values `true` and `false` suggests a boolean type.
          In fact, it is currently documented as boolean. Of course, we can
          change the type to string, but then having a string type with values
          `true`, `false`, and `attachments` is unintuitive and ugly. Instead, we
          could introduce two new query parameter values (e.g. `yes` and
          `with_attachments`) that don't suggest that it's a boolean, while
          deprecating the values `true` and `false`.

          Show
          klaus_trainer Klaus Trainer added a comment - > Would it make sense to share a pattern from the stale/feed parameters > and turn include_docs into a ternary? E.g. `include_docs=true`, > `include_docs=attachments`? Yeah, we would avoid introducing another query parameter by doing so. However, allowing the values `true` and `false` suggests a boolean type. In fact, it is currently documented as boolean. Of course, we can change the type to string, but then having a string type with values `true`, `false`, and `attachments` is unintuitive and ugly. Instead, we could introduce two new query parameter values (e.g. `yes` and `with_attachments`) that don't suggest that it's a boolean, while deprecating the values `true` and `false`.
          Hide
          klaus_trainer Klaus Trainer added a comment -

          Thinking about that, I just found a strong argument in favour of
          Nathan Vander Wilt's original proposal to use the same query parameter name like
          in the document API.

          I'm wondering why we shouldn't have the functionality that is provided
          by other query parameters from the document API in the view and changes
          APIs as well. In fact, I see no reason why we couldn't (and shouldn't)
          make the following query parameters available to be used in combination
          with `include_docs`:

          • attachments
          • att_encoding_info
          • conflicts
          • deleted_conflicts
          • local_seq
          • meta
          • revs
          • revs_info

          Now, thinking of that, the most sensible thing would probably be to just
          stick with the names from the document API.

          What do others think about that?

          Show
          klaus_trainer Klaus Trainer added a comment - Thinking about that, I just found a strong argument in favour of Nathan Vander Wilt 's original proposal to use the same query parameter name like in the document API. I'm wondering why we shouldn't have the functionality that is provided by other query parameters from the document API in the view and changes APIs as well. In fact, I see no reason why we couldn't (and shouldn't) make the following query parameters available to be used in combination with `include_docs`: attachments att_encoding_info conflicts deleted_conflicts local_seq meta revs revs_info Now, thinking of that, the most sensible thing would probably be to just stick with the names from the document API. What do others think about that?
          Hide
          jjs Johannes J. Schmidt added a comment -

          `attachments=true` is fine.

          I think its way better to explain "Use attachments=true to include attachments. On a view, you must also set include_docs=true to ask for the docs with the attachments." rather than "On a view you set include_attachments=true along with include_docs=true to get the attachments in one go whereas on a single document fetch you say attachments=true".

          I prefer less query parameter names with slightly different behaviour which explain themselves logically. Various names all have to be remembered.

          Show
          jjs Johannes J. Schmidt added a comment - `attachments=true` is fine. I think its way better to explain "Use attachments=true to include attachments. On a view, you must also set include_docs=true to ask for the docs with the attachments." rather than "On a view you set include_attachments=true along with include_docs=true to get the attachments in one go whereas on a single document fetch you say attachments=true". I prefer less query parameter names with slightly different behaviour which explain themselves logically. Various names all have to be remembered.
          Hide
          klaus_trainer Klaus Trainer added a comment -

          Great. After rethinking and sleeping over it, I'm now quite confident
          that we should name it `attachments`.

          I've updated my feature branch accordingly and pushed it to github again
          (https://github.com/apache/couchdb/pull/106).

          Thanks Nathan Vander Wilt, Robert Newson, and Johannes J. Schmidt for your ideas, feedback, and
          suggestions!

          Show
          klaus_trainer Klaus Trainer added a comment - Great. After rethinking and sleeping over it, I'm now quite confident that we should name it `attachments`. I've updated my feature branch accordingly and pushed it to github again ( https://github.com/apache/couchdb/pull/106 ). Thanks Nathan Vander Wilt , Robert Newson , and Johannes J. Schmidt for your ideas, feedback, and suggestions!
          Hide
          klaus_trainer Klaus Trainer added a comment -

          I just noticed that my previous pull request's branch name and commit
          message were both out of date.

          I therefore closed it and opened a new pull request:
          https://github.com/apache/couchdb/pull/109

          Show
          klaus_trainer Klaus Trainer added a comment - I just noticed that my previous pull request's branch name and commit message were both out of date. I therefore closed it and opened a new pull request: https://github.com/apache/couchdb/pull/109
          Hide
          klaus_trainer Klaus Trainer added a comment -

          I've closed my previous pull request (again, sorry) and created a new
          one that also accommodates `att_encoding_info`:
          https://github.com/apache/couchdb/pull/111

          Show
          klaus_trainer Klaus Trainer added a comment - I've closed my previous pull request (again, sorry) and created a new one that also accommodates `att_encoding_info`: https://github.com/apache/couchdb/pull/111
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit ca41964b70b96b5fa76504e4001523ed29c718b5 in branch refs/heads/master from Klaus Trainer
          [ https://git-wip-us.apache.org/repos/asf?p=couchdb.git;h=ca41964 ]

          Extend support for attachment-related query params

          Until now, the boolean query parameters `attachments` and
          `att_encoding_info` have only been supported for the document API
          endpoint (`/

          {db}/{docid}`).

          This extends support for queries to the changes (`/{db}

          /_changes`) and
          view (`/

          {db}

          /_design/

          {ddoc}

          /_view/

          {view}

          `) API endpoints:

          • If `include_docs` and `attachments` equal `true`, the Base64-encoded
            contents of attachments are included with the documents in changes or
            view query results, respectively.
          • If `include_docs` and `att_encoding_info` equal `true`, encoding
            information is included in attachment stubs if the particular
            attachment is compressed.

          Closes COUCHDB-1923.

          Show
          jira-bot ASF subversion and git services added a comment - Commit ca41964b70b96b5fa76504e4001523ed29c718b5 in branch refs/heads/master from Klaus Trainer [ https://git-wip-us.apache.org/repos/asf?p=couchdb.git;h=ca41964 ] Extend support for attachment-related query params Until now, the boolean query parameters `attachments` and `att_encoding_info` have only been supported for the document API endpoint (`/ {db}/{docid}`). This extends support for queries to the changes (`/{db} /_changes`) and view (`/ {db} /_design/ {ddoc} /_view/ {view} `) API endpoints: If `include_docs` and `attachments` equal `true`, the Base64-encoded contents of attachments are included with the documents in changes or view query results, respectively. If `include_docs` and `att_encoding_info` equal `true`, encoding information is included in attachment stubs if the particular attachment is compressed. Closes COUCHDB-1923 .
          Hide
          klaus_trainer Klaus Trainer added a comment -

          Reopening in order to correctly specify "Fix version/s" when closing it again.

          Show
          klaus_trainer Klaus Trainer added a comment - Reopening in order to correctly specify "Fix version/s" when closing it again.

            People

            • Assignee:
              Unassigned
              Reporter:
              natevw Nathan Vander Wilt
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development