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

Improve description of field masks in API documentation #475

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

benolayinka
Copy link
Contributor

@benolayinka benolayinka commented Jul 20, 2021

Summary

Closes https://github.com/TheThingsIndustries/lorawan-stack-support/issues/373
Closes https://github.com/TheThingsIndustries/lorawan-stack-support/issues/527

Screenshots

Bildschirmfoto 2021-08-05 um 12 45 48

Bildschirmfoto 2021-07-20 um 17 42 45

Bildschirmfoto 2021-07-20 um 17 42 51

Changes

  • Add missing messages and enums to API docs
  • Properly describe field mask usage, add warnings
  • Add that identifiers in body are not required if they are specified in url

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Run Locally: Verified that the docs build using make server, posted screenshots, verified external links. Test with HUGO_PARAMS_SEARCH_ENABLED=true if style changes will affect the search bar.
  • New Features Marked: Documentation for new features is marked using the new-in-version shortcode, according to the guidelines in CONTRIBUTING.
  • Style Guidelines: Documentation obeys style guidelines in CONTRIBUTING.
  • Commits: Commit messages follow guidelines in CONTRIBUTING, there are no fixup commits left.

@benolayinka benolayinka added this to the 2021 Q3 milestone Jul 20, 2021
@benolayinka benolayinka self-assigned this Jul 20, 2021
@github-actions github-actions bot added the needs/triage We still need to triage this label Jul 20, 2021
@NicolasMrad NicolasMrad removed the needs/triage We still need to triage this label Jul 20, 2021
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

I'm afraid this "field mask only" label will make things even more confusing than they already are. As an example of this, your first screenshot shows the Applications message (for list responses) and indicates the applications field as needing to be in the field mask, which isn't the case.

Also, do we emphasize enough that empty fields (zero values) are omitted from the JSON, regardless of field mask? This is of course something we'd want to fix in a future version of The Things Stack but that's going to take some time.

@benolayinka
Copy link
Contributor Author

I'm afraid this "field mask only" label will make things even more confusing than they already are. As an example of this, your first screenshot shows the Applications message (for list responses) and indicates the applications field as needing to be in the field mask, which isn't the case.

Can we somehow get a better list of which fields are field mask only, then? @johanstokking shared this but I don't understand how it's used https://github.com/TheThingsNetwork/lorawan-stack/blob/v3.14/pkg/ttnpb/field_mask_validation.go

@htdvisser
Copy link
Contributor

Not really, because it also depends on the context.

  • The ApplicationRegistry.Create RPC takes an Application in its request, but there is no field mask, it just looks at all fields.
  • The ApplicationRegistry.Get and ApplicationRegistry.List RPCs take a request message with a field mask to indicate the fields to return.
  • The ApplicationRegistry.Update takes both an Application and a field mask that indicates which fields to update.

I think the best way to document this, is to do it in the request messages, so in the GetApplicationRequest for example, we would document that the ids, created_at, updated_at fields are always selected. In entity messages (such as Application) we would then say that the fields that are returned/updated are selected in the request message.

@benolayinka benolayinka force-pushed the doc/api-improvements branch from 2d6e5b8 to b8d3a39 Compare August 2, 2021 21:11
@benolayinka benolayinka changed the title API Improvements Improve description of field masks in API documentation Aug 2, 2021
@benolayinka benolayinka requested a review from htdvisser August 2, 2021 21:12
@benolayinka benolayinka force-pushed the doc/api-improvements branch from b8d3a39 to 6eb4bc6 Compare August 2, 2021 21:16
@benolayinka
Copy link
Contributor Author

Not really, because it also depends on the context.

  • The ApplicationRegistry.Create RPC takes an Application in its request, but there is no field mask, it just looks at all fields.
  • The ApplicationRegistry.Get and ApplicationRegistry.List RPCs take a request message with a field mask to indicate the fields to return.
  • The ApplicationRegistry.Update takes both an Application and a field mask that indicates which fields to update.

I think the best way to document this, is to do it in the request messages, so in the GetApplicationRequest for example, we would document that the ids, created_at, updated_at fields are always selected. In entity messages (such as Application) we would then say that the fields that are returned/updated are selected in the request message.

Created a new issue for this -> #493

Will merge this as is, with improved description of how field masks work

{{% tts %}} APIs use field masks to specify a subset of fields that should be returned by a reading request, or to specify fields that should be updated in a writing request. See Google's [Protocol Buffers reference](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.FieldMask) for more information about field masks.
{{% tts %}} APIs use field masks to specify a subset of fields that should be returned by a reading request, or to specify fields that should be updated in a writing request. By default, API requests will not return or update most fields, unless they are specified in a field mask.

The following fields are always returned and always updated:
Copy link
Contributor

Choose a reason for hiding this comment

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

These fields are always returned for messages that have them.
Only updated_at is automatically updated.

@@ -6,17 +6,185 @@ weight: -1

## Field Masks

{{% tts %}} APIs use field masks to specify a subset of fields that should be returned by a reading request, or to specify fields that should be updated in a writing request. See Google's [Protocol Buffers reference](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.FieldMask) for more information about field masks.
{{% tts %}} APIs use field masks to specify a subset of fields that should be returned by a reading request, or to specify fields that should be updated in a writing request. By default, API requests will not return or update most fields, unless they are specified in a field mask.
Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on the request. Not all requests take field masks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benolayinka benolayinka requested a review from htdvisser August 4, 2021 23:01
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

I still think we can do better, but let's not block this any longer.

{{< warning >}} If you are not getting the fields you expect in API responses, see the [Field Masks]({{< relref "field-mask" >}}) reference.
{{</ warning >}}

{{< note >}} If you're having trouble with the HTTP API, you can always inspect requests in the Console using your browser's inspector. All of the data displayed in the Console is pulled using HTTP API requests, and this should give you some insight in to how they are formed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{< note >}} If you're having trouble with the HTTP API, you can always inspect requests in the Console using your browser's inspector. All of the data displayed in the Console is pulled using HTTP API requests, and this should give you some insight in to how they are formed.
{{< note >}} If you are having trouble with the HTTP API, you can always inspect requests in the Console using your browser's inspector. All of the data displayed in the Console is pulled using HTTP API requests, and this should give you some insight in to how they are formed.

@benolayinka benolayinka merged commit 5a835dc into master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants