Skip to content

fix: update template with noop returned undefined template #11688

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

Merged
merged 5 commits into from
Jan 19, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jan 18, 2024

Fixes #11186

When the UI submits an update to a template, and there has been no change, the response is a 304 (Not modified) with an empty payload. The UI expected a response, and returned /undefined as the template name.

The unit test added was just testing the reported case, which turned out not to be an issue.

Create a template, remove everyone group, see if owner can read
Emyrk added 2 commits January 18, 2024 11:13
The patch response did not include the template. The UI required the
template to be returned to form the new page path
@Emyrk Emyrk changed the title test: add unit test to excercise owner reading template bug fix: update template with noop returned undefined template Jan 18, 2024
Comment on lines +691 to +693
// Create a template, remove the group, see if an owner can
// still fetch the template.
t.Run("GetOnEveryoneRemove", func(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably worth keeping, it was the original report.

@Emyrk Emyrk marked this pull request as ready for review January 18, 2024 17:31
@Emyrk Emyrk requested a review from Parkreiner January 18, 2024 18:35
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

I think the client-side fix makes sense – just had one suggestion for a possible improvement.

I don't think I should be trusted with the Go stuff, though. I hate to delegate that part, but I'm still getting up to speed on the language, and am a little worried that some nuances might be lost on me

Comment on lines 32 to 37
// This update has a chance to return a 304 which means nothing was updated.
// In this case, the return payload will be empty and we should use the
// original template data.
if (!data) {
data = template;
}
Copy link
Member

@Parkreiner Parkreiner Jan 18, 2024

Choose a reason for hiding this comment

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

I think this change works for our purposes here. I guess there's just two other things that come to mind:

  1. At the end of the day, we do need an undefined check, but right now, there's nothing surfacing that the data could be undefined at the type level.

    My worry is that someone will see that we're doing a check for something that appears non-nullable, and not be sure whether the comment still applies. Could you also add an undefined union for updateTemplateMeta?

    I know that we're using this API call in two other files, though, so there might be some edge cases with changing it everywhere. onSuccess's data argument could also be broadened to include undefined as a bandaid solution

export const updateTemplateMeta = async (
  templateId: string,
  data: TypesGen.UpdateTemplateMeta,
): Promise<TypesGen.Template | undefined> => {
  const response = await axios.patch<TypesGen.Template | undefined>(
    `/api/v2/templates/${templateId}`,
    data,
  );
  return response.data;
};
  1. I haven't dived too deeply into our settings stuff, but my gut feeling is that we don't necessarily need to invalidate the queries and go through a refetch if no data has changed. I wonder if something like this might be good:
onSuccess: async (data) => {
  if (!data) {
    data = template;
  } else {
    await queryClient.invalidateQueries(
      templateByNameKey(orgId, data.name),
    );
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all good info and I will make these changes. Other places call this, but have to handle the undefined too.

@Emyrk Emyrk requested review from BrunoQuaresma and Parkreiner and removed request for BrunoQuaresma January 18, 2024 19:28
@Emyrk
Copy link
Member Author

Emyrk commented Jan 18, 2024

I did attempt to solve this in the backend, but I could not.

Golang does not allow writing a body for some status codes:

https://github.com/golang/go/blob/master/src/net/http/server.go#L1569-L1574

Comment on lines 31 to 38
onSuccess: async (data) => {
// This update has a chance to return a 304 which means nothing was updated.
// In this case, the return payload will be empty and we should use the
// original template data.
if (!data) {
data = template;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix.

Comment on lines 32 to 37
// This update has a chance to return a 304 which means nothing was updated.
// In this case, the return payload will be empty and we should use the
// original template data.
if (!data) {
data = template;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is all good info and I will make these changes. Other places call this, but have to handle the undefined too.

): Promise<TypesGen.Template> => {
const response = await axios.patch<TypesGen.Template>(
): Promise<TypesGen.Template | undefined> => {
const response = await axios.patch<TypesGen.Template | undefined>(
Copy link
Member

@Parkreiner Parkreiner Jan 19, 2024

Choose a reason for hiding this comment

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

Oh wait, I just realized something – I'm worried the logic isn't quite right. undefined isn't a valid JSON-serializable value (though null is), so it shouldn't be able to make it across the API boundary in the first place

Is the condition we should actually be checking for whether data.name is the string literal "undefined"? Even in the bug issue, the previous code wasn't breaking when it was trying to access the name property during the navigation – it was just being fed an incorrect value. That's making me think that technically, data and its properties would always be defined – it's just that we're simulating "undefined-ness" by setting name to "undefined" in certain situations

This is definitely out of scope for the PR, and I haven't explored our type generation logic, so I don't know how complicated this would be, but there is a trick we can use to add the literal "undefined" to autocomplete without narrowing the overall type unnecessarily:

interface Template {
  readonly name: "undefined" | (string | {}); // (string | NonNullable<unknown>) also works
}

With that, "undefined" would show up during autocomplete, but the overall type would still be string

Copy link
Member Author

Choose a reason for hiding this comment

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

The response is a completely empty payload, 0 bytes.

You are saying that is null and not undefined?

null is more explicit, and harder to make occur by mistake.
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

👍

@Emyrk Emyrk enabled auto-merge (squash) January 19, 2024 18:51
@Emyrk Emyrk disabled auto-merge January 19, 2024 18:54
@Emyrk Emyrk merged commit ca48b87 into main Jan 19, 2024
@Emyrk Emyrk deleted the stevenmasley/owner_read_template branch January 19, 2024 18:54
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Owner role can't update a template, if template's Everyone role removed
2 participants