Skip to content

SDWAN-252 | authz version semver #1

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.

Sign up for GitHub

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

Merged
katzoded merged 10 commits into from
Apr 11, 2023
Merged

SDWAN-252 | authz version semver #1

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
31fb88c
1. changed each version-indentifier-part to be a uint64_t instead of …
katzoded Apr 11, 2023
9f01d50
first develop commit
katzoded Apr 11, 2023
3c4a057
1. changed each version-indentifier-part to be a uint64_t instead of…
katzoded Apr 11, 2023
810fa9f
1. changed each version-indentifier-part to be a uint64_t instead of…
katzoded Apr 11, 2023
d78bc56
add code owners
katzoded Apr 11, 2023
28aad5a
remove redundant
katzoded Apr 11, 2023
46c2a5c
remove the exception part from the parse function
katzoded Apr 11, 2023
a8479bd
refactor parsing function name
katzoded Apr 11, 2023
3bedf65
address PR comments
katzoded Apr 11, 2023
07ccb14
address PR comments
katzoded Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 35 additions & 23 deletions include/semver/semver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,29 @@ SOFTWARE.

namespace semver
{

typedef uint64_t BuildIdentifierPart;
Copy link

@stablebits stablebits Apr 11, 2023

Choose a reason for hiding this comment

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

minor: using is considered to be more in line with modern C++ than typedef. e.g. using BuildIdentifierPart = uint64_t;.

Copy link
Author

@katzoded katzoded Apr 11, 2023

Choose a reason for hiding this comment

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

the idea was to allow users of this class to define the size of BuildIdentifierPart before the include (if someone wants a smaller size)

but for some reason I couldn't make it work...
I'll change it to using


const std::string default_prerelease_part = "0";
const char prerelease_delimiter = '.';
const std::string numbers = "0123456789";
const std::string prerelease_allowed_chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-";
const std::string version_pattern = "^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$";
const std::string loose_version_pattern = "^v?(0|[1-9]\\d*)(?:\\.(0|[1-9]\\d*))?(?:\\.(0|[1-9]\\d*))?(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$";
const std::string version_pattern = "^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)"
Copy link

@stablebits stablebits Apr 11, 2023

Choose a reason for hiding this comment

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

Why do we change regex? Where are these new patterns coming from?

Copy link

@loganbauer loganbauer Apr 11, 2023

Choose a reason for hiding this comment

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

It's the same regex, Oded just broke it into lines to make it easier to read the major.minor.patch parts.

Copy link
Author

@katzoded katzoded Apr 11, 2023

Choose a reason for hiding this comment

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

@logan is right, I just broke the regex into those parts as defined in standard.

"(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))"
Copy link
Author

@katzoded katzoded Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@Twingate/sdwan,
maybe you can help me understand this regex and if it complies with RFC ABNF .

<pre-release identifier> ::= <alphanumeric identifier>
                           | <numeric identifier>

I would do something much easier on this one (?:-[a-zA-Z0-9\.]*), although I tried to modify it and it didn't go that easy for the tests!

Copy link
Author

@katzoded katzoded Apr 11, 2023

Choose a reason for hiding this comment

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

actually the regex it taken from RFC

"?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$";
const std::string loose_version_pattern = "^v?(0|[1-9]\\d*)(?:\\.(0|[1-9]\\d*))?(?:\\.(0|[1-9]\\d*))"
"?(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))"
"?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$";

struct semver_exception : public std::runtime_error {
semver_exception(const std::string& message) : std::runtime_error(message) { }
};

inline BuildIdentifierPart parse_build_identifier_part(const std::string& version_part)
{
return (BuildIdentifierPart)std::stoull(version_part);
Copy link

@stablebits stablebits Apr 11, 2023

Choose a reason for hiding this comment

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

minor: static_cast<BuildIdentifierPart>(...) is also more like modern C++ ))

}

inline std::vector<std::string> split(const std::string& text, const char& delimiter) {
std::size_t pos_start = 0, pos_end, delim_len = 1;
std::string current;
Expand Down Expand Up @@ -69,7 +81,7 @@ namespace semver
private:
bool m_numeric = false;
std::string m_value;
unsigned long m_numeric_value;
BuildIdentifierPart m_numeric_value;
public:
prerelease_part(const std::string& part) {
if (part.empty()) {
Expand All @@ -81,7 +93,7 @@ namespace semver
throw semver_exception(
"Pre-release part '" + part + "' is numeric but contains a leading zero.");
}
m_numeric_value = std::stoul(part);
m_numeric_value = parse_build_identifier_part(part);
m_numeric = true;
}
if (!is_valid_prerelease(part)) {
Expand All @@ -93,7 +105,7 @@ namespace semver

bool numeric() const { return m_numeric; }
std::string value() const { return m_value; }
unsigned long numeric_value() const { return m_numeric_value; }
BuildIdentifierPart numeric_value() const { return m_numeric_value; }

int compare(const prerelease_part& other) const {
if (m_numeric && !other.m_numeric) return -1;
Expand Down Expand Up @@ -193,9 +205,9 @@ namespace semver

class version {
private:
unsigned long m_major;
unsigned long m_minor;
unsigned long m_patch;
BuildIdentifierPart m_major;
BuildIdentifierPart m_minor;
BuildIdentifierPart m_patch;
prerelease_descriptor m_prerelease;
std::string m_build_meta;

Expand All @@ -212,9 +224,9 @@ namespace semver
return 0;
}
public:
version(unsigned long major = 0,
unsigned long minor = 0,
unsigned long patch = 0,
version(BuildIdentifierPart major = 0,
BuildIdentifierPart minor = 0,
BuildIdentifierPart patch = 0,
std::string prerelease = "",
std::string build_meta = "")
: m_major{major},
Expand All @@ -223,9 +235,9 @@ namespace semver
m_prerelease{prerelease_descriptor::parse(prerelease)},
m_build_meta{build_meta} { }

unsigned long major() const { return m_major; }
unsigned long minor() const { return m_minor; }
unsigned long patch() const { return m_patch; }
BuildIdentifierPart major() const { return m_major; }
BuildIdentifierPart minor() const { return m_minor; }
BuildIdentifierPart patch() const { return m_patch; }
std::string prerelease() const { return m_prerelease.str(); }
std::string build_meta() const { return m_build_meta; }

Expand Down Expand Up @@ -302,9 +314,9 @@ namespace semver
static version parse(const std::string& version_str, bool strict = true) {
std::regex regex(strict ? version_pattern : loose_version_pattern);
std::cmatch match;
unsigned long major;
unsigned long minor;
unsigned long patch;
BuildIdentifierPart major;
BuildIdentifierPart minor;
BuildIdentifierPart patch;
std::string prerelease = "";
std::string build_meta = "";

Expand All @@ -323,13 +335,13 @@ namespace semver

try {
if (strict && major_m.matched && minor_m.matched && patch_m.matched) {
major = std::stoul(major_m);
minor = std::stoul(minor_m);
patch = std::stoul(patch_m);
major = (BuildIdentifierPart) parse_build_identifier_part(major_m);
minor = (BuildIdentifierPart) parse_build_identifier_part(minor_m);
patch = (BuildIdentifierPart) parse_build_identifier_part(patch_m);
} else if (!strict && major_m.matched) {
major = std::stoul(major_m);
minor = minor_m.matched ? std::stoul(minor_m) : 0;
patch = patch_m.matched ? std::stoul(patch_m) : 0;
major = (BuildIdentifierPart) parse_build_identifier_part(major_m);
minor = minor_m.matched ? (BuildIdentifierPart) parse_build_identifier_part(minor_m) : 0;
patch = patch_m.matched ? (BuildIdentifierPart) parse_build_identifier_part(patch_m) : 0;
Copy link

@stablebits stablebits Apr 11, 2023

Choose a reason for hiding this comment

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

parse_buld_identifier_part() already returns BuildIdentifierPart so we don't need these type casts.

Copy link
Author

@katzoded katzoded Apr 11, 2023

Choose a reason for hiding this comment

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

right, good catch... its a leftovers

} else {
throw semver_exception("Invalid version: " + version_str);
}
Expand Down
1 change: 1 addition & 0 deletions test/version.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ TEST_CASE("Test version strings", "[version]") {
REQUIRE(semver::version::parse("1", false).str() == "1.0.0");
REQUIRE(semver::version::parse("1.2", false).str() == "1.2.0");
REQUIRE(semver::version::parse("v1.2", false).str() == "1.2.0");
REQUIRE(semver::version::parse("18446744073709551615.18446744073709551614.18446744073709551613").str() == "18446744073709551615.18446744073709551614.18446744073709551613");

REQUIRE(semver::version::parse("v1.2.3-alpha+build", false).str() == "1.2.3-alpha+build");
REQUIRE(semver::version::parse("v1-alpha+build", false).str() == "1.0.0-alpha+build");
Expand Down