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

fix: handle nil pointer dereference in Defaulter.getEntitySchema #904

Closed
wants to merge 1 commit into from
Closed

fix: handle nil pointer dereference in Defaulter.getEntitySchema #904

wants to merge 1 commit into from

Conversation

tjasko
Copy link
Member

@tjasko tjasko commented May 3, 2023

Corrects a nil pointer dereference when Defaulter.getEntitySchema() is called & the HTTP request fails.

@tjasko tjasko requested a review from a team May 3, 2023 19:33
@CLAassistant
Copy link

CLAassistant commented May 3, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (02b90d6) 35.58% compared to head (a9c2df5) 35.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #904      +/-   ##
==========================================
- Coverage   35.58%   35.57%   -0.01%     
==========================================
  Files          92       92              
  Lines       11278    11279       +1     
==========================================
  Hits         4013     4013              
- Misses       6872     6873       +1     
  Partials      393      393              
Impacted Files Coverage Δ
utils/defaulter.go 49.65% <0.00%> (-0.18%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@aboudreault aboudreault left a comment

Choose a reason for hiding this comment

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

looking into the test failure..

@GGabriele
Copy link
Collaborator

Due to the nature of the API, I think we should actually do something like this: #869

@@ -215,12 +215,15 @@ func (d *Defaulter) getEntitySchema(entityType string) (map[string]interface{},
return schema, err
}
resp, err := d.client.Do(d.ctx, req, &schema)
if err != nil {
Copy link
Contributor

@aboudreault aboudreault May 4, 2023

Choose a reason for hiding this comment

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

I think we want to preserve the behavior described here: https://github.com/Kong/deck/pull/904/files#diff-d85c74f21d13d17a0932f304636914d8aacd18acb50f86632aee600dd1080b20R221

looks like we are receiving a *kong.APIError with the 404 code, so we could check this instead of checking the resp.StatusCode.

Copy link
Member Author

@tjasko tjasko May 4, 2023

Choose a reason for hiding this comment

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

@GGabriele already has PR up for this that I missed (#869). The Client.Do() method is hooked up in a bit of an abnormal way, so that threw me for a loop. Closing this PR out as such.

@tjasko tjasko closed this May 4, 2023
@tjasko tjasko deleted the fix/get-entity-schema-panic branch May 4, 2023 17:55
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.

None yet

5 participants