feat(core) implement entity tagging #4275

fffonion · 2019-02-01T23:36:21Z

This PR adds support for list entities by combination of tags, and listing all entity IDs with a specific tag.

Summary

Handling of tags per entity is done in different way depending on the strategy. For either strategy, a tags table is created to store tags for entities, each entity table also have tags column.

With Postgres, the tags column of an entity table is indexed with GIN index type:
- When selecting entities by tags, the strategy queries the tags column with Postgres array operator, which will hit index.
- When doing insert/update/upsert/delete, the strategy set the tags column of the table and update the columns in tags table as well. It uses trigger to ensure consistency.
With cassandra:
- When selecting entities by tags, the strategy queries the tags table with corresponding entity_id and entity_name, then select from the entity table with the returned entity_ids.
- When doing insert/update/upsert/delete, the strategy set the tags column of the table and update the columns in tags table as well. It uses the cassandra:batch function to ensure atomicity and consistency.

For a custom entity that needs tags functionality, developer will need to set tags field to be true in the schema, add a tags field to fields defination, and add migrations as kong/db/migrations/core/003_100_to_110.lua.

Full changelog

Implement storing tags for entities and listing entities by combination of tags.
Implement listing all tags or specific tag with related entity UUIDs.
Implement the batch function in the Cassandra connector.

kong/api/endpoints.lua

bungle · 2019-02-07T12:45:09Z

kong/api/endpoints.lua

+        options.tags_cond = 'or'
+        options.tags = split(tags, '/')
+      else
+        options.tags = {}


In this case, should the:

options.tags = tags -- or options.tags = args.tags

So that later on, when we validate, we have original faulty input there, which can make a better error message?

bungle · 2019-02-07T12:48:29Z

kong/db/dao/init.lua

+    elseif type(options.tags) ~= "table" then
+      errors.tags = "must be a table"
+    elseif #options.tags > 5 then
+      errors.tags = "cannot be query more than 5 tags"


cannot be query more than 5 tags -> cannot query more than 5 tags

kong/api/endpoints.lua

hishamhm

@fffonion This is an amazing piece of work! It does cover a lot:

Core:
1.1. a pretty regular-looking tags attribute in core entity schemas (which triggers special behavior in the DAO when modified), which is currently injected by the magic tags = true
1.2. a read-only Tags entity (which is only updated via side-effects on the tags attribute of other entities), used to support the page_by_tags
1.3. new method options opts.tags and opts.tags_cond which filter on core entity searches
API:
2.1. new queryargs ?tags=foo,bar and ?tags=foo/bar (I hope we don't need to support / in tag names in the future!)
2.2. new endpoint /tags/:tags

I sprinkled comments all over, but it's mostly typos and missing tests (e.g. 2.2). We need to have more tests for the failing cases. And the Tags DAO object needs to be protected from destructive operations.

At this point I wanted to avoid "designy" questions as I think the design and implementation are pretty well rounded at this point. The only thing sticking out to me that I couldn't help but bring up is the magic tags = true attribute, because at this point I think we don't really need it:

If it was added to prevent tags from being incorrectly specified in schemas: the codebase also assumes a particular format for id and that doesn't have special construction; the schema for Entities in kong.db.schema.entity could double-check it if necessary.
If it was added to make it easy to test if a schema is tags-enabled: schema.fields.tags works just as well as schema.tags.

Maybe this design stems from earlier when tags were more special and less of a regular field? They are currently a regular field in schemas, in DB tables, in the DAO object... we totally could do without special casing it in the schema level.

I think that wouldn't be a significant change at this point (it would be more removing code than adding), but I think it would be an improvement. I'm actually more concerned about the DAO protections and missing tests; I feel these would give us extra confidence for the merge.

kong/db/dao/init.lua

hishamhm · 2019-02-07T14:33:08Z

kong/db/dao/init.lua

+    elseif not match(concat(options.tags), "^[%w%.%-%_~]+$") then
+      errors.tags = "must only contain alphanumeric and '., -, _, ~' characters"
+    elseif #options.tags > 1 and options.tags_cond ~= "and" and options.tags_cond ~= "or" then
+      errors.tags_cond = "must be a either 'and' or 'or' when more than one tags are specified"


more than one tags are specified -> more than one tag is specified

hishamhm · 2019-02-07T14:44:24Z

kong/db/strategies/postgres/tags.lua

+
+local sql_templates = {
+  page_first = [[
+  SELECT entity_id, entity_name, tag, ordinarity 


typo? ordinarity

hishamhm · 2019-02-07T14:48:04Z

spec/02-integration/03-db/07-tags_spec.lua

+      end
+    end)
+
+    it("list all entities attached with tag", function()


list all entities attached with tag

this doesn't list all entities attached with a given tag, but rather "list all entities that have tags", right? If so, the latter could be a clearer description for the test.

yes, have tags will describe it clearer

spec/02-integration/03-db/07-tags_spec.lua

spec/02-integration/04-admin_api/14-tags_spec.lua

kong/api/endpoints.lua

spec/02-integration/03-db/07-tags_spec.lua

hishamhm · 2019-02-07T16:02:33Z

kong/db/dao/init.lua

@@ -145,6 +146,24 @@ local function validate_options_value(options, schema, context)
    errors.ttl = fmt("cannot be used with '%s'", schema.name)
  end

+  if schema.tags == true and options.tags ~= nil then
+    if context ~= "select" then


does the options.tags filter also work in generated methods for relationships, such as db.targets:each_for_upstream({ id = U.id }, nil, { tags = T })? (i.e. "give me all targets of upstream X that have tag T") — there needs to be a test in the suite answering this question :)

each_for_* will not support tags because it will not parse "?tags=" from Admin API, and uses a different pager page_for_* other than the tag-enabled function page used by normal pagination. I'll add a unit test on each_for_upstream to ensure the functionality doesn't exist.

hishamhm · 2019-02-07T16:09:42Z

spec/02-integration/04-admin_api/14-tags_spec.lua

+local helpers = require "spec.helpers"
+local cjson = require "cjson"
+
+-- We already test the functionality of page() when filtering by tag in 


there are no tests for /tags?

what happens with /tags with no arguments

what happens with /tags/valid_tag

what happens with /tags/invalid@tag

what happens when mixing /tags and ?tags=

what happens with /tags/valid_tag?tags=another_tag

what happens with /tags/valid_tag?tags=another_tag,yet_another

what happens with /tags/valid_tag?tags=another_tag/yet_another

what happens with /tags/valid_tag?tags=invalid@tag

anything else?

Added tests for /tags and /tags/:tags in spec/02-integration/04-admin_api/14-tags_spec.lua 👍

kong/db/dao/tags.lua

kong/db/strategies/postgres/init.lua

kong/db/strategies/postgres/tags.lua

james-callahan · 2019-02-08T07:35:28Z

spec/02-integration/03-db/02-db_core_entities_spec.lua

@@ -487,6 +487,7 @@ for _, strategy in helpers.each_strategy() do
            regex_priority  = 0,
            preserve_host   = false,
            strip_path      = true,
+            tags            = ngx.null,


Should we return an empty set rather than null?

This will follow the same pattern as for example paths for routes, which if empty will be set to ngx.null

Those are different though: adding a path constricts the behaviour: it says "only consider this route when one of these array elements match" -> an empty path array would be effectively turning a route off.
Tags are different: an empty list of tags is the same as no tags, and so IMO should always be an array.
In practice this lets us remove lots of type checks, both inside of Kong, and in users of Kong.

@james-callahan while I agree in principle that we should have been using empty sets rather than nulls everywhere (and I even disagree about your sub-point that paths are rightfully null, one could easily pick a semantics where empty set means no constraint added to paths), but this battle was fought in 2017 and lost (Ctrl+F "billion"), and now null is the default in all aggregate empty-accepting fields.

So, in short, @fffonion's design is fine, as it is consistent with the behavior of other attributes.

fffonion · 2019-02-12T00:06:44Z

Since the changes that address the above comments are small and sparse, i'll make them a single commit. Once this PR looks good I'll squash them into previous commit for better commit history.

hishamhm

I just reviewed the new commits and left a couple minor suggestions, but I'm happy with the current status of the PR. I think it looks good and once the latest changes made are squashed into their respective commits, I think it's a go.

hishamhm · 2019-02-12T13:10:03Z

kong/db/schema/entities/tags.lua

@@ -7,7 +7,7 @@ return {
  dao          = "kong.db.dao.tags",

  fields = {
-    { tag          = { type = "string", required = true, unique = false }, },
+    { tag          = { type = "string", required = true, unique = false, match = "^[%w%.%-%_~]+$" }, },


super minor suggestion so that the definition of a valid tag comes from a single source of truth:

{ tag = typedefs.tag { required = true } }

with a definition of tag in typedefs.lua below

hishamhm · 2019-02-12T13:12:00Z

kong/db/schema/typedefs.lua

+    type = "string",
+    match = "^[%w%.%-%_~]+$"
+  },
+}


typedefs.tag = Schema.define { type = "string", match = "^[%w%.%-%_~]+$" } typedefs.tags = Schema.define { type = "set", elements = typedefs.tag, }

so you can reuse the definition of tag in the schema of the tags entity.

hishamhm · 2019-02-12T13:24:00Z

spec/02-integration/04-admin_api/14-tags_spec.lua

+            username = "adminapi-filter-by-tag-" .. i,
+            tags = { "corp_a",  "consumer"..i }
+          }
+          local row, err, err_t = bp.consumers:insert(consumer)


A minor request that I'm sure will sound like it came out of nowhere: please insert items with bp before you start_kong. Besides making tests less dependent on event propagation in general, this actually will make the test compatible with declarative config! (I have a magic bp in my branch that builds up a config.yml to be used by start_kong when using database=off :) )

hishamhm · 2019-02-12T13:32:51Z

kong/db/dao/tags.lua

@@ -1,11 +1,27 @@
 local Tags = {}

 function Tags:page_by_tag(tag, size, offset, options)
+  local ok, err = self.schema:validate_field(self.schema.fields.tag, tag)


fffonion · 2019-02-12T19:48:37Z

Hi, I have squashed commits. The tests are passing expect one https://travis-ci.org/Kong/kong/jobs/492317694, but I'm not sure how to understand this error.

Review comments have been addressed

hishamhm · 2019-02-13T12:29:21Z

@fffonion Merged! \o/

bungle previously requested changes Feb 7, 2019

View reviewed changes

hishamhm suggested changes Feb 7, 2019

View reviewed changes

james-callahan previously requested changes Feb 8, 2019

View reviewed changes

hishamhm reviewed Feb 12, 2019

View reviewed changes

fffonion force-pushed the feat/core-entity-tags branch 3 times, most recently from 3e10256 to a90ccd1 Compare February 12, 2019 19:47

fffonion added 5 commits February 12, 2019 21:41

feat(db) add tags field to core entities

343925e

feat(db) add support for filtering and paginating by tag

dbe2679

feat(api) add support for filtering by tags

7ce073a

feat(tags) add the tags entity

1071cf2

tests(tags) add tests for tags and update entity tests with tags

9f69772

hishamhm force-pushed the feat/core-entity-tags branch from a90ccd1 to 9f69772 Compare February 12, 2019 23:43

hishamhm approved these changes Feb 12, 2019

View reviewed changes

hishamhm added this to the 1.1.0 milestone Feb 13, 2019

hishamhm merged commit abcfdf1 into next Feb 13, 2019

hishamhm deleted the feat/core-entity-tags branch February 13, 2019 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core) implement entity tagging #4275

feat(core) implement entity tagging #4275

fffonion commented Feb 1, 2019 •

edited

Loading

bungle Feb 7, 2019

bungle Feb 7, 2019

hishamhm left a comment

hishamhm Feb 7, 2019

hishamhm Feb 7, 2019

hishamhm Feb 7, 2019

fffonion Feb 11, 2019

hishamhm Feb 7, 2019

fffonion Feb 11, 2019

hishamhm Feb 7, 2019

fffonion Feb 12, 2019

james-callahan Feb 8, 2019

fffonion Feb 11, 2019

james-callahan Feb 12, 2019

hishamhm Feb 12, 2019

fffonion commented Feb 12, 2019

hishamhm left a comment

hishamhm Feb 12, 2019

hishamhm Feb 12, 2019

hishamhm Feb 12, 2019

hishamhm Feb 12, 2019

fffonion commented Feb 12, 2019

hishamhm commented Feb 13, 2019

feat(core) implement entity tagging #4275

feat(core) implement entity tagging #4275

Conversation

fffonion commented Feb 1, 2019 • edited Loading

Summary

Full changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamhm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fffonion commented Feb 12, 2019

hishamhm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fffonion commented Feb 12, 2019

hishamhm commented Feb 13, 2019

fffonion commented Feb 1, 2019 •

edited

Loading