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

Fixing null pointer exception on DAO update when config is missing #571

Merged
merged 1 commit into from
Sep 28, 2015

Conversation

subnetmarco
Copy link
Member

Fix for #570.

@subnetmarco subnetmarco changed the title Null pointer check Fixing null pointer exception on DAO update when config is missing Sep 25, 2015
@subnetmarco
Copy link
Member Author

@thibaultcha - this fix seems to work.

@thibaultcha
Copy link
Member

It should probably rather be fixed at the model layer by setting it to an empty object by default.

@subnetmarco
Copy link
Member Author

It should probably rather be fixed at the model layer by setting it to an empty object by default.

By doing so, will the DAO store an empty JSON object (as a result of the serialization of an empty table into a string)?

@thibaultcha
Copy link
Member

Yes, but since a configuration should be mandatory for all plugins, it might be better having an empty objects that handling the null case (this is an example of having to handle the null case, kong.lua is another one)

@subnetmarco
Copy link
Member Author

I am experiencing a weird behavior in my tests. Adding a default = {} to the config property, results in the following test output:

Failurespec/integration/dao/cassandra/base_dao_spec.lua @ 612
Cassandra Base DAO plugins :update() should properly update a plugin with an empty configuration
spec/integration/dao/cassandra/base_dao_spec.lua:623: Expected to be falsy, but value was:
(table) {
  [is_dao_error] = true
  [message] = {
    [config.hide_credentials] = 'hide_credentials is an unknown field'
    [config.key_names] = 'key_names is an unknown field' }
  [schema] = true }

at the following line: https://github.com/Mashape/kong/blob/fix/dao-update/spec/integration/dao/cassandra/base_dao_spec.lua#L623

It seems like the error is completely unrelated since I am explicitly saving a request-transformer plugin 😕

@subnetmarco
Copy link
Member Author

Seems like these lines are affecting the test later on: https://github.com/Mashape/kong/blob/fix/dao-update/spec/integration/dao/cassandra/base_dao_spec.lua#L111-L118

Commenting them out makes everything work.

@thibaultcha
Copy link
Member

Ok I will check this this we

Because the schema validation was expecting a literal, it did not copy
the empty object in case of using it to specify the `default` of a
property, thus leading later unrelated inserted plugins to have invalid
configs.

See test case in entities_schemas_spec.

Fix #570
@thibaultcha
Copy link
Member

Rewrote the PR, see updated changes

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed NEEDS REVIEW labels Sep 27, 2015
subnetmarco added a commit that referenced this pull request Sep 28, 2015
Fixing null pointer exception on DAO update when config is missing
@subnetmarco subnetmarco merged commit 75a7ec7 into master Sep 28, 2015
@subnetmarco subnetmarco deleted the fix/dao-update branch September 28, 2015 19:16
subnetmarco added a commit that referenced this pull request Sep 28, 2015
xvaara added a commit to xvaara/kong that referenced this pull request Oct 3, 2015
* Mashape/master: (23 commits)
  Update README.md
  Update README.md
  Closing Kong#562
  Adding wait time before ratelimiting tests
  Fixing test
  fix(jwt) handle `iss` not being found in jwt credentials
  Update README.md
  docs(update) remove redundancy
  docs(update) fix layout
  fix(test) fix config test after Kong#563
  Update README.md
  Adding missing statement for Kong#571
  perf(analytics) global optimizations
  fix(plugins) make default config for plugins an empty object
  Closes Kong#445
  dbocs(changelog) 0.5.0 changes
  Better content-type check in response-transformer plugin
  Closes Kong#535
  Fixes the root problem at Kong#565
  fix(key-auth) remove support for key in request body
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants