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(key-auth) don't crash when conf.anonymous is nil #2264

Closed
wants to merge 1 commit into from

Conversation

Vermeille
Copy link
Contributor

No description provided.

@Tieske
Copy link
Member

Tieske commented Mar 27, 2017

@Vermeille how did you set the property to nil ? From what I see, it can only be an empty string?

@Vermeille
Copy link
Contributor Author

Not sure. It was nil after I migrated from 0.9.9, and explicitly setting it to "" is not solving anything.

@Tieske
Copy link
Member

Tieske commented Mar 27, 2017

The default value is "", hence the extra test at runtime can be omitted (performance)

@Vermeille
Copy link
Contributor Author

So I thought. But I somehow managed to have this nil value instead of "". Any idea why?

$ curl 127.0.0.1:8002/apis/https-bench-service/plugins/b7ad7b2a-ef5b-4c77-cc91-0a29deb1d7ee | jq
{
  "api_id": "b7ad7b2a-ef5b-4c77-cc91-0a29deb1d7ee",
  "id": "b7ad7b2a-ef5b-4c77-cc91-0a29deb1d7ee",
  "created_at": 1476703979000,
  "enabled": true,
  "name": "key-auth",
  "config": {
    "anonymous": "",
    "key_names": [
      "X-Client-ID"
    ],
    "hide_credentials": false
  }
}

@Tieske
Copy link
Member

Tieske commented Mar 27, 2017

That's because no migration was added to explicitly set missing field to the default "" value.

@Vermeille
Copy link
Contributor Author

I am not sure I'm being clear.
Despite explicitly setting config.anonymous="" via the admin endpoint as you can see above, I still have conf.anonymous == nil

@Tieske
Copy link
Member

Tieske commented Mar 27, 2017

see #2266.

closing this.

@Tieske Tieske closed this Mar 27, 2017
@Tieske
Copy link
Member

Tieske commented Mar 27, 2017

@Vermeille thx for the report!

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