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

Filter version comparison #432

Closed
PengZheng opened this issue Jul 12, 2022 · 5 comments · Fixed by #505
Closed

Filter version comparison #432

PengZheng opened this issue Jul 12, 2022 · 5 comments · Fixed by #505
Assignees

Comments

@PengZheng
Copy link
Contributor

    celix_filter_t *filter = celix_filter_create("(&(service.version>=1.2.2)(service.version<=1.2.2))");
    celix_properties_t *prop = celix_properties_create();
    celix_properties_set(prop, "service.version", "1.2.2.0");
    CHECK_TRUE(celix_filter_match(filter, prop)); // It fails.
    celix_properties_destroy(prop);
    celix_filter_destroy(filter);

It happens because celix_filter makes simple strcmp when dealing with <= and >=.
The above makes our dm_exmaple broken, i.e., phase2b's requirement is never satisfied and thus not active:

#define PHASE1_RANGE_EXACT  "[1.2.2.0,1.2.2.0]"

        celix_dmServiceDependency_setService(dep, PHASE1_NAME, PHASE1_RANGE_EXACT, NULL);
@tira-misu
Copy link
Contributor

In addition [1.5.0,2.0.0) did not match if you version is 1.10.0 . The version comparison uses a simple string compare instead of a version compare. so 1.10.0 is less than 1.5.0, but it should be greater. So at the moment 2 digit version numbers are not supported.

Is this expected behavior or a bug?

@pnoltes
Copy link
Contributor

pnoltes commented Jan 3, 2023

This is a bug and bug that is broader then only version, because filter comparison is always done on strings.
So the order is always lexicographically, even for something like (service.id<20).

I am currently working on this, but this will take a while because it has impact on the properties implementation, properties usage and filter implementation.

@pnoltes pnoltes self-assigned this Jan 3, 2023
@tira-misu
Copy link
Contributor

A more local change would be to only change filter implementation. If both compare items are convertable to a version make a version compare, else make a string compare.

@tira-misu
Copy link
Contributor

i made a quick fix in celix 1.12 for me. Perhaps its a solution for current branch too?

static celix_status_t compareVersion(char* version1, char* version2, int* compareResult) {
  version_pt v1 = NULL;
  version_pt v2 = NULL;

  if (version_createVersionFromString(version1, &v1) != CELIX_SUCCESS ||
      version_createVersionFromString(version2, &v2) != CELIX_SUCCESS)
  {
		return CELIX_ILLEGAL_ARGUMENT;
  }
	version_compareTo(v1, v2, compareResult);
	version_destroy(v1);
	version_destroy(v2);
	return CELIX_SUCCESS;
}
  case EQUAL: {
    if (compareVersion(string, (char*)value2, &versionCompareResult) == CELIX_SUCCESS)
    {
      *result = versionCompareResult == 0;
      return CELIX_SUCCESS;
    }
    *result = (strcmp(string, (char*)value2) == 0);
    return CELIX_SUCCESS;
  }
  case GREATER: {
    if (compareVersion(string, (char*)value2, &versionCompareResult) == CELIX_SUCCESS)
    {
      *result = versionCompareResult > 0;
      return CELIX_SUCCESS;
    }
    *result = (strcmp(string, (char*)value2) > 0);
    return CELIX_SUCCESS;
  }
  case GREATEREQUAL: {
    if (compareVersion(string, (char*)value2, &versionCompareResult) == CELIX_SUCCESS)
    {
      *result = versionCompareResult >= 0;
      return CELIX_SUCCESS;
    }
    *result = (strcmp(string, (char*)value2) >= 0);
    return CELIX_SUCCESS;
  }
  case LESS: {
    if (compareVersion(string, (char*)value2, &versionCompareResult) == CELIX_SUCCESS)
    {
      *result = versionCompareResult < 0;
      return CELIX_SUCCESS;
    }
    *result = (strcmp(string, (char*)value2) < 0);
    return CELIX_SUCCESS;
  }
  case LESSEQUAL: {
    if (compareVersion(string, (char*)value2, &versionCompareResult) == CELIX_SUCCESS)
    {
      *result = versionCompareResult <= 0;
      return CELIX_SUCCESS;
    }
    *result = (strcmp(string, (char*)value2) <= 0);
    return CELIX_SUCCESS;
  }

@pnoltes
Copy link
Contributor

pnoltes commented Jan 8, 2023

Thanks @tira-misu I will take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants