fix: remove non-existent colums in getMembershipFromQuery queries

[doncicuto]

In issue #7841 @mahmoodfathy commented an issue when the API call for Listing My ZITADEL Manager Roles is called with any kind of query (orgQuery, projectQuery, projectGrantQuery...). A column XXXXXX does not exist (SQLSTATE 42703) error is thrown.

The issue was focused in getMembershipFromQuery where filtering queries functions are called: prepareOrgMember, prepareIAMMember, prepareProjectMember and prepareProjectGrantMember

Those functions allow queries for columns that are not members of the table to be queried so I've added a conditional clause to avoid using the queries that cannot be called.

For example, for prepareOrgMember, member.id, member.project_id and member.grant_id columns are not added to the filter queries

for _, q := range query.Queries {
		if q.Col().table.name == membershipAlias.name &&
			!slices.Contains([]string{membershipIAMID.name, membershipProjectID.name, membershipGrantID.name}, q.Col().name) {
			builder = q.toQuery(builder)
		}
	}
	return builder.MustSql()

Here I show one screenshot where the error "column XXXXXX does not exist (SQLSTATE 42703)" is no longer thrown using an orgQuery.

Should close #7841

Definition of Ready

• [X] I am happy with the code
• [X] Short description of the feature/issue is added in the pr description
• [X] PR is linked to the corresponding user story
• [X] Acceptance criteria are met
• [ ] All open todos and follow ups are defined in a new ticket and justified
• [ ] Deviations from the acceptance criteria and design are agreed with the PO and documented.
• [X] No debug or dead code
• [X] My code has no repetitions
• [X] Critical parts are tested automatically
• [ ] Where possible E2E tests are implemented
• [ ] Documentation/examples are up-to-date
• [ ] All non-functional requirements are met
• [X] Functionality of the acceptance criteria is checked manually on the dev system.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 2, 2024 0:08am

[stebenz]

@doncicuto I'm a bit on the fence on this, in my opinion the correct way to solve this would be a bit different, so that you query for the specific columns of the sub-tables.

Would that not be solvable if you use for example

func NewMembershipOrgIDQuery(value string) (SearchQuery, error) {
	return NewTextQuery(OrgMemberOrgID, value, TextEquals)
}

instead of

func NewMembershipOrgIDQuery(value string) (SearchQuery, error) {
	return NewTextQuery(membershipOrgID, value, TextEquals)
}

and then

if q.Col().table.name == membershipAlias.name || q.Col().table.name == orgMemberTable.name

so that you have all shared columns like membershipUserID but also the column of the specific table, instead of

if q.Col().table.name == membershipAlias.name

Does that make sense?

[doncicuto]

Hi @stebenz, I've tried to fix the code with your suggestion.

I've tried to run some manual tests against the API and all seems fine:

I hope I've understood the suggestion correctly, and my apologies as I'll need a lot of time to understand how the backend works so thanks for your time.

[stebenz]

@doncicuto No problem at all. The idea was that if columns are only filled in specific sub-selects, as they are here for each sub-table, the respective query on the sub table only has the where condition. Anyway, could you please also do some tests for combined queries like projectID and orgID in the same query?

[doncicuto]

Sure @stebenz

Just to confirm, as defined by proto file, you can only use one query method in getMembershipFromQuery so I'm not sure how could I add a test like that to user_membership_test.go to check that proto error is returned:

Docs seem to imply that you can use several queries for project, org... but I don't know how can descriptions like "one of type use iam, org id, project id or project grant id" can be added to docs

If you can give me a hint to implement those tests I'd appreciate it

[stebenz]

@doncicuto The query would be like so:

{
  "query": {
    "offset": "0",
    "limit": 100,
    "asc": true
  },
  "queries": [
    {
      "orgQuery": {
        "orgId": "69629023906488334"
      },
    },
    {
      "projectQuery": {
        "projectId": "69629023906488334"
      }
    }
  ]
}

Unfortunately, the docs are wrongly generated, but that was already in issue #5352.

[doncicuto]

I see @stebenz, but my doubt was that the grpc definition for MembershipQuery says that it uses oneof and therefore if I try to use several queries (one OrgQuery and one ProjectQuery) as the example you propose I get an error from GRPC specifying that I can't use more than one query

message MembershipQuery {
    oneof query {
        option (validate.required) = true;

        MembershipOrgQuery org_query = 1;
        MembershipProjectQuery project_query = 2;
        MembershipProjectGrantQuery project_grant_query = 3;
        MembershipIAMQuery iam_query = 4;
    }
}

In summary you wanted me to test combined queries like projectID and orgID but I don't know how I can test that if GRPC says that I can't use combined queries due to the oneof specification

Thanks @stebenz

[eliobischof]

I see @stebenz, but my doubt was that the grpc definition for MembershipQuery says that it uses oneof and therefore if I try to use several queries (one OrgQuery and one ProjectQuery) as the example you propose I get an error from GRPC specifying that I can't use more than one query

message MembershipQuery {
    oneof query {
        option (validate.required) = true;

        MembershipOrgQuery org_query = 1;
        MembershipProjectQuery project_query = 2;
        MembershipProjectGrantQuery project_grant_query = 3;
        MembershipIAMQuery iam_query = 4;
    }
}

In summary you wanted me to test combined queries like projectID and orgID but I don't know how I can test that if GRPC says that I can't use combined queries due to the oneof specification

Thanks @stebenz

@doncicuto Maybe the PR #8002 I just created helps. It explains how to actually use the queries array https://docs-git-docs-please-help-zitadel.vercel.app/docs/apis/known_docs_issues#generated-examples-for-oneof-fields

[stebenz]

Hm, but the queries are a repeated list on the request:

message ListUserMembershipsRequest {
    //list limitations and ordering
    string user_id = 1 [(validate.rules).string = {min_len: 1, max_len: 200}];
    //the field the result is sorted
    zitadel.v1.ListQuery query = 2;
    //criteria the client is looking for
    repeated zitadel.user.v1.MembershipQuery queries = 3;
}
message MembershipQuery {
    oneof query {
        option (validate.required) = true;

        MembershipOrgQuery org_query = 1;
        MembershipProjectQuery project_query = 2;
        MembershipProjectGrantQuery project_grant_query = 3;
        MembershipIAMQuery iam_query = 4;
    }
}

The idea here is that you can have a list of query parameters, where each one is a oneof so that you only can query with specific types.

[doncicuto]

Ok I think I understand you but I don't explain myself so I feel a little dumb here.

My point, you want me to try a test like this:

{
  "query": {
    "offset": "0",
    "limit": 100,
    "asc": true
  },
  "queries": [
    {
      "orgQuery": {
        "orgId": "69629023906488334"
      },
    },
    {
      "projectQuery": {
        "projectId": "69629023906488334"
      }
    }
  ]
}

But if I execute that query against my code I get a GRPC error parsing the query not accepting it, and that error I assume is because the GRPC doesn't like that I include more than one query type and that error is not part of my code.

As always I don't want to waste your time so if you prefer it I can close this PR as I don't know how I can fix that GRPC error to fullfil your requirements.

Thanks!

[stebenz]

The screenshot you sent here, should that contain the example I provided? As I see it you use, with 1 object and 2 attributes "orgQuery" and "projectQuery", where these 2 are the oneof and why you get the error:

{
  "queries": [
    { 
      "orgQuery": { "orgId": "69629023906488334" },
      "projectQuery": { "projectId": "69629023906488334" }
    }
  ]
}

And not use different objects in the array:

{
  "queries": [
      { "orgQuery": { "orgId": "69629023906488334" }},
      { "projectQuery": { "projectId": "69629023906488334" }}
  ]
}

[doncicuto]

Thanks @stebenz, it's clear that I'm dumb enough

[hifabienne]

@doncicuto what is the state on this pr?

[doncicuto]

Hi @hifabienne I've to add the tests that Stefan suggested. On wednesday morning I'll resume my collaboration to the Zitadel project so hopefully I'll update my PRs on the waiting state that day. Sorry that I have to stop working these past weeks but I'm looking for a new paying job as I'm on a sabbatical leave from my civil service job... Anyway I'll have more time for my collaborations

[hifabienne]

Hi @hifabienne I've to add the tests that Stefan suggested. On wednesday morning I'll resume my collaboration to the Zitadel project so hopefully I'll update my PRs on the waiting state that day. Sorry that I have to stop working these past weeks but I'm looking for a new paying job as I'm on a sabbatical leave from my civil service job... Anyway I'll have more time for my collaborations

No worries and no stress, thanks for the update

[doncicuto]

Hi @stebenz, first of all I'm sorry that I haven't made any progress these past weeks. I've tried to resume my work on pending PRs and other issues.

I think I need some help again. Last time we spoke I was stuck working on combined queries and you helped me to understand that I was too blind to see how I should prepare the right query object. I've tried to run the query and I got and empty result:

I think the empty result is fine, as no results can satisfy the where clause:

WHERE members.org_id = '271257922917433736' AND members.project_id = '271257922917499272' AND members.user_id = '271258664688419208' AND members.instance_id = '271257922917368200'"

If I try to test a combine a query with projectId and projectGrantId I get this:

Which seems correct for the where clause:

WHERE members.project_id = '271257922917499272' AND members.grant_id = '271259087843361160'  AND members.user_id = '271258664688419208' AND members.instance_id = '271257922917368200'"

Hope that you find these results fine and expected. In case they don't I should have to continue asking you.

Now, I wanted to add the tests that you requested me.

I understand that you want me to add some combined queries to internal/query/user_membership_test.go to test the prepareMembershipsQuery, right?

I've spent some hours but I don't know how can I add those tests to that file. Should I create a different wrapper like prepareMembershipWrapperForCombinedQueries that accepts a MembershipSearchQuery and should I add an array with the test queries to be performed? Should I create a different membershipsStmt that has the option to pass WHERE arguments to pass the orgId, projectId...? I guess it must be an easy task and I've tried to find an example to avoid bothering you but I'd need a clue, sorry about this

[hifabienne]

hei @doncicuto Thanks for your feedback, as stefan is currently on vaction and you two have discussed already a lot my suggestion is to wait for him to get back. If you do need the feedback earlier let me know, and I can look for someone else.

[doncicuto]

Perfect @hifabienne, I'll wait then and focus on some recent UI Bugs Thanks!

[stebenz]

Perfect @hifabienne, I'll wait then and focus on some recent UI Bugs Thanks!

Hi @doncicuto, you are totally correct as well, I was somehow under the impression that there were integration tests around, which is not the case, as there you could have added the queries with no problems. I think we can leave the tests out until we have this covered in the new API(s), sorry to create a hold up here.

IMO if everything is manually tested with the changes, we can merge it.

[doncicuto]

Thanks @stebenz for your review as always