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(core):fix missing info of flattened_errors #13998

Closed
wants to merge 4 commits into from

Conversation

lhanjian
Copy link
Contributor

@lhanjian lhanjian commented Dec 10, 2024

Summary

When a nested configuration error occurs, a more precise and identifiable description is required
When nested configuration conflicted occurs, it returns the desired flatterned error

Please see the test cases for details of the problem

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix FTI-6351/FTI-6318

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added core/db schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Dec 10, 2024
@lhanjian lhanjian force-pushed the fix/missing-flattened-errors branch from 2e900f2 to 76cb75e Compare December 10, 2024 07:00
@windmgc windmgc requested a review from flrgh December 11, 2024 07:56
@@ -862,6 +863,9 @@ do
-- }
-- }
--
if type(err_t[ref.field]) == "string" then
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the error message is always string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@outsinre maybe nil, or table.

err_t[ref.field] just like field_err_t

errs[entity] = errs[entity] or {}
errs[entity][i] = uniqueness_error_msg(entity, endpoint_key, key)
local errs_item_name = entity
if parent_entity and tostring(parent_idx) then
Copy link
Contributor

Choose a reason for hiding this comment

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

If parent_idx is nil, then tostring(parent_idx) is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@outsinre
i think tostring(nil) maybe nil?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The printed nil is a string, not the Lua nil.

errs_item_name = parent_entity.."|"..tostring(parent_idx).."|"..entity
end
errs[errs_item_name] = errs[errs_item_name] or {}
errs[errs_item_name][i] = uniqueness_error_msg(entity, endpoint_key, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

The errs_item_name and entity on this line refer to the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@outsinre no.

entity is its original name.
errs_item_name is key name of errs

changelog/unreleased/kong/fix-missing-flattened-erros.yml Outdated Show resolved Hide resolved
}
})

local body = assert.response(res).has.status(400)
Copy link
Contributor

@outsinre outsinre Dec 12, 2024

Choose a reason for hiding this comment

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

The 400 error is due to DB-less by default does not support this kind of format. DB-less uses declarative format that needs all entities flattened at top-level.

It expects the snis not nested within the certificates.

it("flatten_errors when conflicting inputs", function()
local conflicting_input = {
_format_version = "3.0",
consumers = {
Copy link
Contributor

@flrgh flrgh Dec 13, 2024

Choose a reason for hiding this comment

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

Hmm 🤔 I don't know if it's possible to properly flatten errors in this case, because the result we get from the declarative validator is ambiguous. Consider this input:

{
  _format_version = "3.0",
  consumers = {
    {
      username = "consumer-01",
      basicauth_credentials = {
        { username = "user-01", password = "pwd" },
        { username = "user-11", password = "pwd" },
        { username = "user-02", password = "pwd" },
        { username = "user-11", password = "pwd" },
      }
    },
    {
      username = "consumer-02",
      basicauth_credentials = {
        { username = "user-99", password = "pwd" },
      }
    },
    {
      username = "consumer-03",
      basicauth_credentials = {
        { username = "user-01", password = "pwd" },
        { username = "user-33", password = "pwd" },
        { username = "user-02", password = "pwd" },
        { username = "user-33", password = "pwd" },
      }
    }
  }
}

It has the following duplicates:

  1. consumers[1].basicauth_credentials[4] (user-11)
  2. consumers[3].basicauth_credentials[1] (user-01)
  3. consumers[3].basicauth_credentials[3] (user-02)
  4. consumers[3].basicauth_credentials[4] (user-33)

The error table we receive from :parse_table() looks like this:

{
  basicauth_credentials = {
    [1] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-01' already declared",
    [3] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-02' already declared",
    [4] = "uniqueness violation: 'basicauth_credentials' entity with username set to 'user-33' already declared",
  },
}
  1. The declarative code actually overwrote the error in consumers[1].basicauth_credentials[4] with the error in consumers[3].basicauth_credentials[4] (presumably because they both happened at index 4)
  2. We cannot use this information to determine which consumer each basicauth_credentials error corresponds to

In summary, I don't think this can be fixed in the error-flattening code alone. A fix within kong.db.schema.others.declarative_config must be made first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flrgh
I try to add your inputs to test case, it can get 4 results.

Is this what you need?

@flrgh
Copy link
Contributor

flrgh commented Dec 13, 2024

I actually fixed a very similar issue back in #12797. The problem is the fix was only applied for primary key errors, whereas the basicauth_credentials.username case is a unique field and not a primary key. I've applied the fix in #14017.

@outsinre
Copy link
Contributor

outsinre commented Dec 16, 2024

There are two errors in this case.

  1. The basicauth_credentials error caused by

    local ok, err = mutually_required(needs_changed and original_input or input, args)
    but swallowed by

    We'd better merge the two errors table.

  2. The snis error caused by

    errors[name] = validation_errors.REQUIRED

The root cause is that DB-less mode expects flattened declarative format as stated here: #13998 (comment).

@flrgh flrgh closed this in #14017 Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/db schema-change-noteworthy size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants