Add version collate functions #8168

Micah-Kolide · 2023-10-20T02:20:07Z

Quality of life improvement. Version strings can be annoying to compare/evaluate since a simple operation can't properly handle numeric values. To work around that I've added a new collation for version comparison.

osquery> SELECT '101.1.10' > '101.1.2';
+------------------------+
| '101.1.10' > '101.1.2' |
+------------------------+
| 0                      |
+------------------------+
osquery> SELECT '101.1.10' > '101.1.2' COLLATE VERSION;
+----------------------------------------+
| '101.1.10' > '101.1.2' COLLATE VERSION |
+----------------------------------------+
| 1                                      |
+----------------------------------------+

This works by splitting strings on . into segments, then grouping consecutive numeric and alphabetic characters into pieces to compare against. Special characters are dropped, but they continue to split off the grouping. I considered merging those split off groups together, but I feel it would be best to continue splitting off to the next piece instead.

This should also be pretty nice on version columns in tables if we add the collation to them.

osquery> CREATE TEMP TABLE test(version TEXT COLLATE VERSION);
osquery> INSERT INTO test VALUES ('100.2.30');
osquery> SELECT * FROM test WHERE version > '100.2.4';
+----------+
| version  |
+----------+
| 100.2.30 |
+----------+
osquery> SELECT version FROM os_version WHERE version > '11.0';
+---------+
| version |
+---------+
| 14.0    |
+---------+
osquery> SELECT version FROM os_version WHERE version > '100.0';
+---------+
| version |
+---------+
| 14.0    |
+---------+
osquery> INSERT INTO test VALUES ('14.0');
osquery> SELECT * FROM test WHERE version > '11.0';
+----------+
| version  |
+----------+
| 100.2.30 |
| 14.0     |
+----------+
osquery> SELECT * FROM test WHERE version > '100.0';
+----------+
| version  |
+----------+
| 100.2.30 |
+----------+

Smjert · 2023-10-20T17:29:53Z

Closing and reopening to pick up a test fix from master.

directionless

I think test files will help. You can root around https://github.com/osquery/osquery/pull/6535/files but I don't like the implementation there. (Which is why I never merged it)

osquery/sql/sqlite_collation.cpp

Smjert · 2023-10-23T12:03:44Z

I think test files will help. You can root around https://github.com/osquery/osquery/pull/6535/files but I don't like the implementation there. (Which is why I never merged it)

By the way, we also have this: https://github.com/osquery/osquery/blob/master/osquery/utils/versioning/semantic.cpp, which is currently unused, so I think we should have one implementation at the end.

EDIT: To clarify, if there's no use of code like semantic.cpp (as it's happening now) which extracts the major, minor, patch values from a string, I would just remove it.

directionless

This looks pretty clean to me, and reasonably easy to follow. (Sweeeet!)

A couple of suggestions:

I kinda understand all the boolean options to versionCompare, but I think some comments would help. Some words about why one might want epoch, and how it's used by dpkg. For example...
If we wanted to expose these as sql functions, and not just collations, how would we? Does it require moving any of the functions around?
Some docs should land in https://github.com/osquery/osquery/blob/master/docs/wiki/introduction/sql.md which will then appear in https://osquery.readthedocs.io/en/latest/introduction/sql/ Maybe a Collation section?
Tests are hard to follow, but I'm not sure how to make them better.It is, inherently, a lot.

osquery/sql/tests/sql.cpp

osquery/sql/sqlite_collation.cpp

Smjert

This is only a quick stylistic review: can we use std::string_view instead of const char* + length?

With that, things like strcspn can be substituted with the string_view method find, which will return the position in the string where the character has been found.

Basically, lets not write C.

The variable names should be snake_case.

Finally, for the switch cases on the ASCII value, instead of the number, can we use the character itself? '~' etc, so that it's easier to understand which character that corresponds to.

directionless · 2023-11-07T11:31:32Z

This is only a quick stylistic review: can we use std::string_view instead of const char* + length?

I think that part is driven by the collate api.

Smjert · 2023-11-07T13:26:21Z

I think that part is driven by the collate api.

That's the interface, but versionCompare can be written using std::string_view (which can be constructed from a char * and length).
In general, I'm not saying to never use a pointer and a length separately, but since conceptually those are strings/part of strings, having a single entity that represents that is better for readability and to avoid errors, vs having to separately keep them correctly in sync.

directionless

Thinking a bit about tests, I have two observations:

Everything is testing true. I wonder if anything should test false
I wonder if it would be more or less clear to define an array and then sort the whole thing and check the order. Doesn't handle the equality case, but would it be easier to see the order in?

osquery/sql/tests/sql.cpp

Micah-Kolide · 2023-11-09T19:19:15Z

This is only a quick stylistic review: can we use std::string_view instead of const char* + length?

I've switched to using string_views in versionCompare, and followed the other formatting suggestions.

I went ahead and made the version_compare function, since it looked pretty easy to add that around versionCompare.

Added information to the documents, and more comments around the versionCompare options.

Everything is testing true. I wonder if anything should test false

I felt there wasn't a need to test false since we can iterate other operators, but I can add some false tests if it would make things more clear.

I wonder if it would be more or less clear to define an array and then sort the whole thing and check the order. Doesn't handle the equality case, but would it be easier to see the order in?

I don't quite know what you mean by this. Could you provide an example?

osquery/sql/sqlite_version.cpp

My inital suggestions have been addressed

docs/wiki/introduction/sql.md

directionless

I think the sql function gets simplified, this is mergable ,

docs/wiki/introduction/sql.md

…r vals and branching collation variations

…nality to support the tests

…rsion compare function

…on collations/function

…an true or false for a certain operator

docs/wiki/introduction/sql.md

directionless

I think this is good. @Smjert anything else?

Micah-Kolide requested review from a team as code owners October 20, 2023 02:20

Smjert closed this Oct 20, 2023

Smjert reopened this Oct 20, 2023

directionless reviewed Oct 23, 2023

View reviewed changes

osquery/sql/sqlite_collation.cpp Outdated Show resolved Hide resolved

osquery/sql/sqlite_collation.cpp Outdated Show resolved Hide resolved

osquery/sql/sqlite_collation.cpp Outdated Show resolved Hide resolved

sharvilshah added the SQL label Oct 26, 2023

Micah-Kolide force-pushed the micah/add_version_comparison branch from d2a39bb to 2b8b227 Compare November 3, 2023 15:12

Micah-Kolide requested review from directionless and Smjert November 3, 2023 18:27

directionless reviewed Nov 7, 2023

View reviewed changes

Smjert previously requested changes Nov 7, 2023

View reviewed changes

directionless reviewed Nov 7, 2023

View reviewed changes

osquery/sql/tests/sql.cpp Show resolved Hide resolved

Micah-Kolide requested review from Smjert and directionless November 9, 2023 19:31

Smjert reviewed Nov 13, 2023

View reviewed changes

osquery/sql/sqlite_version.cpp Outdated Show resolved Hide resolved

directionless added this to the 5.11.0 milestone Nov 25, 2023

Micah-Kolide mentioned this pull request Dec 8, 2023

Add support for additional collations in column definitions #8214

Merged

directionless reviewed Dec 12, 2023

View reviewed changes

docs/wiki/introduction/sql.md Outdated Show resolved Hide resolved

directionless reviewed Dec 12, 2023

View reviewed changes

docs/wiki/introduction/sql.md Outdated Show resolved Hide resolved

docs/wiki/introduction/sql.md Show resolved Hide resolved

Micah-Kolide added 5 commits December 13, 2023 09:10

Add version compare collate

b27d964

Move collation to own file

f345520

Add collation register function

762eada

Add collation file to cmake list

0a2ac5e

Make if tree simpler

2824ca2

Micah-Kolide added 15 commits December 13, 2023 09:10

re-factor into more modular parts centered around comparing ASCII cha…

45f8642

…r vals and branching collation variations

Add debian, rpm, and arch version collations

89acf2b

Refactor version collations

6293c66

Added version collate test cases and finished version compare functio…

1d8420d

…nality to support the tests

Simplify remainder_precedence if blocks

0142bce

Updated comment for comp_remaining swap blocks

7afd4aa

Remove versioning semantic

b3489ae

Updated version compare methods and formatting, and added a sqlite ve…

df26a16

…rsion compare function

Added comments, version compare tests, and documentation around versi…

0d51880

…on collations/function

Add explation to version collate test

146733f

Include vector

d72dd11

format code

eddb88a

Pass args by value instead of reference in compare functions

9fa703f

Update version compare func to return versionCompare result rather th…

2bbccb3

…an true or false for a certain operator

Format code

d771b28

Micah-Kolide force-pushed the micah/add_version_comparison branch from 9964283 to d771b28 Compare December 13, 2023 17:22

Update version compare func tests

787396e

Micah-Kolide requested review from directionless and Smjert December 13, 2023 17:37

directionless reviewed Dec 13, 2023

View reviewed changes

docs/wiki/introduction/sql.md Outdated Show resolved Hide resolved

Update compare flavor to accept a string

238d003

Micah-Kolide requested a review from directionless December 14, 2023 16:02

directionless approved these changes Dec 14, 2023

View reviewed changes

directionless changed the title ~~Add version collate~~ Add version collate functiona Dec 18, 2023

directionless changed the title ~~Add version collate functiona~~ Add version collate functions Dec 18, 2023

directionless merged commit a184da4 into osquery:master Dec 18, 2023
16 checks passed

directionless mentioned this pull request Feb 18, 2024

Add a SQLite function for comparing version strings #5196

Closed

noahtalerman mentioned this pull request Jul 25, 2024

Policy automations: install software fleetdm/fleet#19551

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add version collate functions #8168

Add version collate functions #8168

Micah-Kolide commented Oct 20, 2023

Smjert commented Oct 20, 2023 •

edited

Loading

directionless left a comment

Smjert commented Oct 23, 2023 •

edited

Loading

directionless left a comment

Smjert left a comment •

edited

Loading

directionless commented Nov 7, 2023

Smjert commented Nov 7, 2023 •

edited

Loading

directionless left a comment

Micah-Kolide commented Nov 9, 2023 •

edited

Loading

directionless left a comment

directionless left a comment

Add version collate functions #8168

Add version collate functions #8168

Conversation

Micah-Kolide commented Oct 20, 2023

Smjert commented Oct 20, 2023 • edited Loading

directionless left a comment

Choose a reason for hiding this comment

Smjert commented Oct 23, 2023 • edited Loading

directionless left a comment

Choose a reason for hiding this comment

Smjert left a comment • edited Loading

Choose a reason for hiding this comment

directionless commented Nov 7, 2023

Smjert commented Nov 7, 2023 • edited Loading

directionless left a comment

Choose a reason for hiding this comment

Micah-Kolide commented Nov 9, 2023 • edited Loading

directionless left a comment

Choose a reason for hiding this comment

directionless left a comment

Choose a reason for hiding this comment

Smjert commented Oct 20, 2023 •

edited

Loading

Smjert commented Oct 23, 2023 •

edited

Loading

Smjert left a comment •

edited

Loading

Smjert commented Nov 7, 2023 •

edited

Loading

Micah-Kolide commented Nov 9, 2023 •

edited

Loading