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

Don't encode %24 in docs #1149

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Don't encode %24 in docs #1149

merged 3 commits into from
Jun 3, 2024

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Jun 1, 2024

I noticed that there's a mix of literal $ and %-encoded %24 used. The %24 are not rendered correctly. See for example the request table for https://docs.getodk.org/central-api-odata-endpoints/#data-document

What has been done to verify that this works as intended?

Rendered using make api-docs (🫡 @sadiqkhoja), spot checked and saw $ as expected.

Why is this the best possible solution? Were any other approaches considered?

I considered modifying the docs code to decode values but given that several literal $ already exist and that literal values work without replacement, that didn't feel like the best option.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Could break some docs.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@matthew-white
Copy link
Member

The %24 are not rendered correctly.

saw $ as expected.

I think %24 was there intentionally with the thought that $ should be encoded in query strings. But you're right that there's currently a mix of %24 and $. I've also never had trouble with using $ when using the API. We usually use $ instead of %24 in Backend tests.

I think the docs are more readable with $ instead of %24. There are other places where we don't encode query strings. For example, for $filter=__system/submissionDate le 2020-01-31, the / and space should be encoded, but I think it's fine the way it is. I think it's on the API user to encode query strings as needed. If it ever becomes confusing, we can always make a note of it.

docs/api.yaml Outdated
@@ -2263,8 +2263,6 @@ paths:
content:
application/json:
schema:
type: object
description: Standard Response
Copy link
Member

Choose a reason for hiding this comment

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

More of a question than anything else, but I don't see a top-level description in the Project schema. Did you remove it here because it just doesn't add much? Have you noticed any trend whereby we should generally be adding description in more places or removing it in more places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should have explained this, sorry. I used https://editor-next.swagger.io/ to see whether the $ might give a syntax error because I thought that might be what the encoding was about. It turns out there are some not small number of errors so I started picking off a couple thinking maybe more would show up at the bottom. That wasn't the case but I committed these anyway.

Those siblings of $ref are ignored by standards-compliant tools. I now realize that our target client is our own docs which are not really spec-compliant so we can keep this and it will get rendered.

I feel so much empathy for the folks dealing with this seemingly-simple limitation that is made difficult to address because of broader specifications. OAI/OpenAPI-Specification#1514 swagger-api/swagger-editor#1184

@matthew-white
Copy link
Member

I think %24 was there intentionally

Meaning I think we intended to display %24 as %24. But I agree that it's better as $.

@lognaturel
Copy link
Member Author

I think %24 was there intentionally with the thought that $ should be encoded in query strings

I see! That actually makes a lot more sense than I thought. Hmm, like you say, it does feel like it's up to the user to do their own escaping.

@lognaturel lognaturel changed the title Docs fixes Don't encode %24 in docs Jun 3, 2024
@lognaturel lognaturel changed the title Don't encode %24 in docs Don't encode $ in docs Jun 3, 2024
@lognaturel lognaturel changed the title Don't encode $ in docs Don't encode %24 in docs Jun 3, 2024
@lognaturel lognaturel merged commit 6eeb5c5 into getodk:master Jun 3, 2024
1 check passed
@lognaturel lognaturel deleted the docs-fixes branch June 3, 2024 22:44
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

Successfully merging this pull request may close these issues.

2 participants