Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 5 commits intomainfromstevenmasley/owner_read_template
Jan 19, 2024

Conversation

Emyrk
Copy link
Member

@EmyrkEmyrk commentedJan 18, 2024
edited
Loading

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
The patch response did not include the template. The UI required thetemplate to be returned to form the new page path
@EmyrkEmyrk changed the titletest: add unit test to excercise owner reading template bugfix: update template with noop returned undefined templateJan 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
MemberAuthor

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.

@EmyrkEmyrk marked this pull request as ready for reviewJanuary 18, 2024 17:31
Copy link
Member

@ParkreinerParkreiner left a comment
edited
Loading

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

@ParkreinerParkreinerJan 18, 2024
edited
Loading

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 anundefined 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 anundefined union forupdateTemplateMeta?

    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'sdata argument could also be broadened to includeundefined as a bandaid solution

exportconstupdateTemplateMeta=async(templateId:string,data:TypesGen.UpdateTemplateMeta,):Promise<TypesGen.Template|undefined>=>{constresponse=awaitaxios.patch<TypesGen.Template|undefined>(`/api/v2/templates/${templateId}`,data,);returnresponse.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{awaitqueryClient.invalidateQueries(templateByNameKey(orgId,data.name),);}}

Copy link
MemberAuthor

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.

@EmyrkEmyrk requested review fromBrunoQuaresma andParkreiner and removed request forBrunoQuaresmaJanuary 18, 2024 19:28
@Emyrk
Copy link
MemberAuthor

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
MemberAuthor

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
MemberAuthor

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

@ParkreinerParkreinerJan 19, 2024
edited
Loading

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 (thoughnull 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 whetherdata.name is the string literal"undefined"? Even in the bug issue, the previous code wasn't breaking when it was trying to access thename 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 settingname 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:

interfaceTemplate{readonlyname:"undefined"|(string|{});// (string | NonNullable<unknown>) also works}

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

Copy link
MemberAuthor

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 isnull and notundefined?

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

@ParkreinerParkreiner left a comment

Choose a reason for hiding this comment

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

👍

@EmyrkEmyrkenabled auto-merge (squash)January 19, 2024 18:51
@EmyrkEmyrkdisabled auto-mergeJanuary 19, 2024 18:54
@EmyrkEmyrk merged commitca48b87 intomainJan 19, 2024
@EmyrkEmyrk deleted the stevenmasley/owner_read_template branchJanuary 19, 2024 18:54
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 19, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@ParkreinerParkreinerParkreiner approved these changes

Assignees

@EmyrkEmyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Owner role can't update a template, if template'sEveryone role removed
2 participants
@Emyrk@Parkreiner

[8]ページ先頭

©2009-2025 Movatter.jp