Fix: Multitenancy support for ORG #7155
Open
+782
−836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 7017e82 in 4 minutes and 27 seconds
More details
- Looked at
3071lines of code in48files - Skipped
0files when reviewing. - Skipped posting
43drafted comments based on config settings.
1. pkg/query-service/app/parser.go:470
- Draft comment:
Ensure that the parseTime and parseTimeMinusBufferStr functions correctly convert and validate timestamps for multitenancy; review for consistency with OrgID usage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. pkg/query-service/app/server.go:295
- Draft comment:
Using 's.serverOptions.Jwt' correctly enforces retrieval of OrgID in JWT claims; review that the middleware 'getUserFromRequest' returns error if OrgID is missing. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. pkg/query-service/auth/auth.go:89
- Draft comment:
Check error usage in obtaining admin user: 'err' not re-assigned before wrapping in error on line 81. Consider using a local variable for error handling from claims extraction. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. pkg/sqlmigration/012_modify_org_domain.go:43
- Draft comment:
Ensure that renaming and dropping columns works properly on all supported DB dialects; consider checking for error messages that might be returned by non-sqlite dialects. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. pkg/sqlmigration/013_update_organization.go:55
- Draft comment:
Ensure that when copying old data into the new table 'apdex_settings_new', the query handles cases where no organization is present. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. pkg/types/organization.go:17
- Draft comment:
Verify that the constraints CHECK conditions for boolean fields 'IsAnonymous' and 'HasOptedUpdates' are properly enforced by the database. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. pkg/types/organization.go:9
- Draft comment:
The new AuditableModel uses time.Time for CreatedAt/UpdatedAt. Ensure that consumers (e.g. API responses and migrations) correctly convert these values (e.g., to Unix timestamps) to avoid breaking existing clients. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that consumers correctly convert time values, which violates the rule against asking for confirmation or ensuring behavior. It doesn't provide a specific suggestion or point out a specific issue with the code.
8. pkg/query-service/auth/auth.go:492
- Draft comment:
Typo detected in the function call: 'GeneratePassowrd' is likely a misspelling of 'GeneratePassword'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/sqlmigration/012_modify_org_domain.go:43
- Draft comment:
Renaming and re-adding the 'updated_at' column in org_domains: Confirm that SQLite (or your target DB) supports DROP COLUMN. Consider a fallback for older versions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/sqlmigration/013_update_organization.go:55
- Draft comment:
Retrieving the org ID uses error message substring matching, which is brittle. Consider checking against a specific error type or code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While using string matching for errors is generally not best practice, this is a one-off migration script that will run once to update the database schema. The error handling here is simple and clear - it's just checking if there are no rows yet. For a migration script, this level of error handling is probably sufficient. More robust error typing would add complexity without much benefit.
The comment raises a valid point about code quality and robustness. String matching can break if error messages change.
Given this is a one-time migration script, the simpler approach is acceptable. The risk of error messages changing is low since this runs against specific database drivers.
While technically correct, the comment suggests additional complexity that isn't warranted for a one-time migration script. The current error handling is clear and sufficient.
11. pkg/sqlmigration/013_update_organization.go:86
- Draft comment:
When adding new columns and migrating data, errors are suppressed by checking for 'duplicate' in the error string. This method may be unreliable; consider a more robust solution if possible. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code is a database migration script that needs to be idempotent. Using string matching for error handling is a common pattern in migration scripts. While not ideal, it's pragmatic given that different databases return different error messages. The alternative would be complex dialect-specific error handling which might be overkill for a migration script.
The comment raises a valid point about error handling robustness. Different database versions or locales could potentially return different error messages.
However, this is a migration script where simplicity and readability are important, and the current approach is a widely used pattern that works well enough for this specific use case.
While technically correct, the comment suggests a complex solution to a simple problem where the current approach is adequate for the specific use case of a one-time migration script.
12. pkg/query-service/auth/rbac.go:56
- Draft comment:
Ensure that GetUserFromReqContext validates OrgID and GroupID properly to enforce multitenancy. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to ensure that a function validates certain IDs properly. It is not a specific code suggestion or a request for a test, and it falls under the rule of not asking the author to ensure behavior is intended or tested.
13. pkg/sqlmigration/009_add_pats.go:49
- Draft comment:
Review the creation of the personal_access_tokens table; ensure that any dependencies on these tokens are isolated since ingestion keys will be removed soon. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/sqlmigration/012_modify_org_domain.go:112
- Draft comment:
Conversion functions (migrateIntToTimestamp and migrateIntToBoolean) rely on implicit type assumptions. Thoroughly test these migrations on production schemas to prevent data loss or conversion issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. ee/query-service/app/server.go:351
- Draft comment:
Typographical inconsistency: the error message still uses 'orgId' even though the field is now 'OrgID' for the user. Consider updating the error message to 'orgID is missing in the claims' for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
16. ee/query-service/auth/auth.go:44
- Draft comment:
Typographical Inconsistency: On line 44, the field is set using 'pat.Id' whereas elsewhere the identifier uses the 'ID' form (e.g., GroupID in line 43). Consider renaming 'pat.Id' to 'pat.ID' for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
17. ee/query-service/dao/sqlite/auth.go:34
- Draft comment:
Typo detected: 'utils.GeneratePassowrd()' appears to be misspelled. It should likely be 'utils.GeneratePassword()'. Please correct this spelling mistake. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. ee/query-service/model/domain.go:57
- Draft comment:
Typo: In the comment for ValidNew, 'cheks' should be corrected to 'checks'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. ee/query-service/model/domain.go:76
- Draft comment:
Typo/Error: In the error message of LoadConfig, the text 'failed to marshal json to OrgDomain{}' should likely be 'failed to unmarshal json to OrgDomain{}' since json.Unmarshal is used. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
20. frontend/src/container/OrganizationSettings/Members/index.tsx:218
- Draft comment:
Typo: The title for the delete modal (line 218) is incorrect. Currently, it reads 'Edit member details' whereas it should reflect that this modal is for deleting a member, e.g., 'Delete member details'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. pkg/query-service/app/http_handler.go:0
- Draft comment:
There are multiple occurrences of the misspelled variable name 'errQuriesByName' (and similar variants like 'errQuriesByNameFetchLatency'). It should be spelled as 'errQueriesByName' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
22. pkg/query-service/app/http_handler.go:4551
- Draft comment:
The variable name 'lastestConfig' appears to be a typo. It should be 'latestConfig' to match standard spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
23. pkg/query-service/app/http_handler.go:1289
- Draft comment:
In function testRule, the variable 'apiRrr' seems to be a typo. It likely should be 'apiErr' for consistency with the rest of the code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
24. pkg/query-service/app/parser.go:1037
- Draft comment:
Typographical error: The variable 'thirdPartApis' should be renamed to 'thirdPartyApis' for consistency with the imported package name (thirdPartyApi). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
25. pkg/query-service/app/server.go:302
- Draft comment:
Typographical issue: In the error message you use "orgId is missing in the claims", but elsewhere you check the field 'OrgID'. To improve clarity and consistency, please update the error message to use the same capitalization (e.g., "OrgID is missing in the claims"). - Reason this comment was not posted:
Comment was on unchanged code.
26. pkg/query-service/auth/auth.go:492
- Draft comment:
Typo: 'GeneratePassowrd' should be corrected to 'GeneratePassword' for clarity and correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
27. pkg/query-service/auth/rbac.go:76
- Draft comment:
Typographical error: Please change 'atleast' to 'at least' in the error message to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
28. pkg/query-service/auth/rbac.go:26
- Draft comment:
Minor grammatical correction in the comment: consider changing 'reads the DB and initialize the auth cache' to 'reads the DB and initializes the auth cache'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
29. pkg/query-service/dao/interface.go:34
- Draft comment:
Inconsistent parameter naming: 'orgId' (in GetUsersByOrg, line 34) is inconsistent with 'orgID' (in GetApdexSettings, line 37). Consider standardizing the naming convention (e.g., always use 'orgID') for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
30. pkg/query-service/dao/interface.go:41
- Draft comment:
Consider renaming the parameter 'sourceUrl' to 'sourceURL' in PrecheckLogin (line 41) for consistency with standard URL capitalization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
31. pkg/query-service/dao/sqlite/connection.go:21
- Draft comment:
There is a minor typographical issue in the comment on line 21: 'InitDB sets up setting up the connection pool global variable.' It would be clearer if rephrased to remove the repetition (e.g., 'InitDB sets up the connection pool global variable.'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
32. pkg/query-service/dao/sqlite/rbac.go:127
- Draft comment:
Minor typographical error in error message: consider changing "Found multiple org with same ID" to "Found multiple orgs with the same ID" for grammatical correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
33. pkg/query-service/dao/sqlite/rbac.go:564
- Draft comment:
Typographical/grammatical suggestion: Instead of "Multiple entries for reset token is found", consider revising it (e.g., "Found multiple entries for reset token" or "Multiple entries for reset token were found") to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
34. pkg/query-service/telemetry/telemetry.go:537
- Draft comment:
Typographical inconsistency: The comment mentions 'user.groupId' whereas the actual field is 'user.GroupID'. Please update the comment to reflect the correct casing. - Reason this comment was not posted:
Comment was on unchanged code.
35. pkg/query-service/telemetry/telemetry.go:565
- Draft comment:
Typographical error: The comment 'Updating a groups properties' should be corrected to "Updating a group's properties" to properly convey possession. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
36. pkg/query-service/tests/integration/filter_suggestions_test.go:286
- Draft comment:
Typographical note: The error message on line 286 says 'could not unmarshal apiResponse.Data json into PipelinesResponse', which appears to be a copy-paste mistake. Since the expected type is v3.QBFilterSuggestionsResponse, consider updating the error message for accuracy. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
37. pkg/query-service/tests/integration/logparsingpipeline_test.go:646
- Draft comment:
Typographical error: Please change 'atleast' to 'at least' in the error message on line 646. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
38. pkg/query-service/tests/integration/logparsingpipeline_test.go:770
- Draft comment:
Typographical error: Please correct 'mistmatch' to 'mismatch' in the error message on line 770. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
39. pkg/query-service/tests/integration/signoz_integrations_test.go:358
- Draft comment:
Typo: The comment at line 358 reads "Integration dashboards should not longer appear in dashboard list after uninstallation". Consider changing it to "should no longer appear". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
40. pkg/query-service/tests/integration/signoz_integrations_test.go:402
- Draft comment:
Typographical issue: The error message at around line 402 begins with an extra space and uses 'json' in lowercase. Consider revising it to remove the extra space and possibly use 'JSON' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
41. pkg/query-service/tests/integration/signoz_integrations_test.go:495
- Draft comment:
Typographical issue: The error message at around line 495 starts with an extra space and uses 'json' in lowercase. It would read better if reformatted (e.g. removing the extra space and using 'JSON'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
42. pkg/sqlmigration/013_update_organization.go:86
- Draft comment:
Typo in comment: "organization, users table already have created_at column" should probably be "organizations, users tables already have a created_at column" or similar for correct grammar. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The rules state we should not make purely informative comments. This is a minor grammatical fix that doesn't impact code functionality or maintainability. While technically correct, fixing grammar in comments is not an important code change that requires attention in a PR review.
The grammar issue could make the comment slightly harder to understand for non-native English speakers. Poor documentation can impact maintainability.
While clear documentation is important, this grammatical issue is minor and the meaning is still understandable. This type of nitpick creates noise in PR reviews.
The comment should be deleted as it's a minor grammatical suggestion that doesn't impact code quality or functionality.
43. pkg/sqlmigration/013_update_organization.go:168
- Draft comment:
Typo in comment: "if the columns is integer then do this" should be corrected to "if the column is integer then do this". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While the comment is technically correct about the typo, it's an extremely minor issue in a comment that doesn't affect functionality. The meaning is still clear even with the typo. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues.
The typo could be seen as reducing code quality and readability. Multiple small issues can add up to make code less professional.
While code quality is important, this is an extremely minor issue in a comment that doesn't impact understanding. The cost of the review comment outweighs the benefit.
Delete this comment as it points out an unimportant typo that doesn't impact code functionality or understanding.
Workflow ID: wflow_UvNSGsQOtXirXMde
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3b8a7e5 in 2 minutes and 30 seconds
More details
- Looked at
113lines of code in1files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. pkg/sqlmigration/013_update_organization.go:39
- Draft comment:
Good refactor: encapsulating apdex settings migration into updateApdexSettings improves readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. pkg/sqlmigration/013_update_organization.go:106
- Draft comment:
Ensure proper handling if organizations table is empty; using LIMIT 1 may hide cases with no org data. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already handles the empty case properly: 1) It checks for "no rows" error and continues 2) It has an explicit if orgID != "" check 3) This is a migration script, so it's reasonable to expect at most one organization in a fresh install. The comment seems to be raising a theoretical concern that's already addressed.
The comment could be pointing out that we should handle multiple organizations differently, or that LIMIT 1 could select an arbitrary org when multiple exist.
Since this is a migration script for a fresh install or upgrade, having exactly 0 or 1 organization is the expected state. The code handles both cases correctly.
The comment should be deleted as it raises a theoretical concern that's already properly handled in the code, and the LIMIT 1 usage is appropriate for this migration context.
3. pkg/sqlmigration/013_update_organization.go:127
- Draft comment:
Consider verifying the logic of copying data from apdex_settings to apdex_settings_new for org scope; ensure it meets multitenancy requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the logic and ensure it meets certain requirements. This falls under asking the author to double-check things, which is against the rules.
4. pkg/sqlmigration/013_update_organization.go:227
- Draft comment:
Update comment correctly reflects conversion to boolean (not timestamp), ensuring clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. pkg/sqlmigration/013_update_organization.go:50
- Draft comment:
Using string matching for duplicate column errors (e.g. checking 'duplicate') can be fragile. Consider checking for specific error codes if supported. - Reason this comment was not posted:
Comment was on unchanged code.
6. pkg/sqlmigration/013_update_organization.go:227
- Draft comment:
The updated comment now correctly states conversion from int to boolean. Verify that the underlying logic exactly matches the intended data conversion. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
7. pkg/sqlmigration/013_update_organization.go:106
- Draft comment:
The new updateApdexSettings function cleanly encapsulates the migration logic for the Apdex settings, complying with the multitenancy update. Note that it selects a single org ID; ensure this approach is revisited once multiple organizations are supported. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. pkg/sqlmigration/013_update_organization.go:177
- Draft comment:
Typo in comment: Consider changing 'if the columns is integer then do this' to 'if the column is integer then do this' for grammatical correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/sqlmigration/013_update_organization.go:80
- Draft comment:
Minor grammatical issue: In the comment on line 80, consider revising 'since organizations, users has created_at as integer instead of timestamp' to 'since organizations and users have created_at as an integer instead of a timestamp' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_HOjmaJH2wjsPCUtu
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0f19e7e in 1 minute and 46 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/sqlmigration/013_update_organization.go:124
- Draft comment:
Good change: comparing directly to sql.ErrNoRows is more robust than string matching. Ensure that an empty orgID is handled correctly downstream. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. pkg/sqlmigration/013_update_organization.go:81
- Draft comment:
Typographical error: In the comment on line 81, 'users has created_at as integer instead of timestamp' should be 'users have created_at as integer instead of timestamp'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. pkg/sqlmigration/013_update_organization.go:178
- Draft comment:
Typographical error: The comment on line 178 says 'if the columns is integer then do this'. It should be updated to 'if the column is integer then do this' for grammatical correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_oTfjOlFNeehvcN9a
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d1e6c40 in 53 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. pkg/sqlmigration/013_update_organization.go:6
- Draft comment:
Addition of the errors import is correct and used below with errors.Is for handling sql.ErrNoRows. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. pkg/sqlmigration/013_update_organization.go:6
- Draft comment:
The addition of the 'errors' import is appropriate since it's now used (e.g., with errors.Is for handling sql.ErrNoRows). Ensure all error comparisons use errors.Is for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. pkg/sqlmigration/013_update_organization.go:56
- Draft comment:
The comment on line 56 says "// organization, users table already have created_at column". It would be clearer if rephrased to something like "// organizations and users tables already have a created_at column". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. pkg/sqlmigration/013_update_organization.go:82
- Draft comment:
The comment on line 82 reads "// since organizations, users has created_at as integer instead of timestamp". Consider revising it to "// since organizations and users have created_at as an integer instead of a timestamp" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. pkg/sqlmigration/013_update_organization.go:179
- Draft comment:
The comment on line 179 states "// if the columns is integer then do this". It would be more accurate to say "// if the column is integer then do this". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_XWpoYLmySfsOADAy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
grandwizard28
requested changes
Feb 21, 2025
Comment on lines
+9
to
+14
| type AuditableModel struct { | ||
| CreatedAt time.Time `bun:"created_at" json:"createdAt"` | ||
| CreatedBy string `bun:"created_by" json:"createdBy"` | ||
| UpdatedAt time.Time `bun:"updated_at" json:"updatedAt"` | ||
| UpdatedBy string `bun:"updated_by" json:"updatedBy"` | ||
| } |
There was a problem hiding this comment.
We should separate this into 2:
- One struct containing
CreatedAtandUpdatedAt. (call it TimeAuditable) - One struct containing
CreatedByandUpdatedBy(call it UserAuditable)
For example:
Organizationneeds only struct 1.Groupprobably needs struct 1. (need to check this and confirm whether 2 will make sense)Userneeds only struct 1.
There was a problem hiding this comment.
- organization settings can be updated by someone no.
- Group needs both. right now we treat group as role, but it should have both.
- same for user, a user role can be updated so it should have both.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f991e73 in 2 minutes and 43 seconds
More details
- Looked at
208lines of code in4files - Skipped
0files when reviewing. - Skipped posting
15drafted comments based on config settings.
1. pkg/query-service/dao/interface.go:15
- Draft comment:
Ensure that adding orgID as the first parameter in all invite-related queries is consistently enforced throughout the implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to ensure consistency in the implementation regarding the use of orgID as the first parameter in invite-related queries. It is not a specific code suggestion or a request for a test, but rather a general request for consistency. This violates the rule against asking the author to ensure certain behaviors are enforced.
2. pkg/query-service/dao/sqlite/rbac.go:26
- Draft comment:
The new orgID filter looks good; please double-check consistency of orgID filtering in both GetInviteFromEmail and GetInviteFromToken. Also consider adding more descriptive error messages when multiple records are returned. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%
The comment asks the PR author to double-check consistency, which violates the rule against asking for confirmation or double-checking. However, the suggestion to add more descriptive error messages is specific and actionable, which aligns with the rules.
3. pkg/query-service/auth/auth.go:65
- Draft comment:
The error message “failed to org id from context” is unclear. Consider changing it to “failed to extract org ID from context”. - Reason this comment was not posted:
Marked as duplicate.
4. pkg/query-service/auth/auth.go:300
- Draft comment:
Similarly, the error message in ValidateInvite when claims extraction fails should be improved to "failed to extract org ID from context" instead of the current wording. - Reason this comment was not posted:
Marked as duplicate.
5. pkg/query-service/dao/interface.go:15
- Draft comment:
The interface changes now include orgID parameters (e.g. in GetInviteFromEmail). The changes are consistent and support multitenancy. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. pkg/query-service/dao/sqlite/rbac.go:38
- Draft comment:
The SQL queries now include filtering by orgID. The changes look correct and consistent; no issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. pkg/query-service/app/http_handler.go:2110
- Draft comment:
Typographical error: The error message currently reads "failed to org id from context". Consider updating it to "failed to get org id from context" for clarity. - Reason this comment was not posted:
Marked as duplicate.
8. pkg/query-service/app/http_handler.go:4863
- Draft comment:
Typographical error: The variable 'errQuriesByName' is misspelled. It should be renamed to 'errQueriesByName' for consistency and clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/query-service/app/http_handler.go:4947
- Draft comment:
Typographical error: The comment mentions "experssion are executed in clickhouse directly". It should read "expressions are executed in clickhouse directly". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. pkg/query-service/app/http_handler.go:111
- Draft comment:
Typographical error: The comment for SetupCompleted says "when the first user is registers." It should be updated to "when the first user is registered." - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/query-service/auth/auth.go:67
- Draft comment:
Typo in error message: "failed to org id from context". Consider updating it to "failed to get org id from context" or "failed to extract org id from context" for clarity. - Reason this comment was not posted:
Marked as duplicate.
12. pkg/query-service/auth/auth.go:506
- Draft comment:
Typographical error: 'GeneratePassowrd' should be corrected to 'GeneratePassword' in the call to utils.GeneratePassowrd(). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. pkg/query-service/dao/interface.go:34
- Draft comment:
Typographical consistency: The parameter here is named 'orgId', whereas elsewhere in the file it is 'orgID'. Consider renaming 'orgId' on line 34 to 'orgID' to ensure consistency across the code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. pkg/query-service/dao/sqlite/rbac.go:151
- Draft comment:
Typographical/lexicographical issue: In the GetOrgByName function, the error message says "Multiple orgs with same ID found". Since the query is by name, it would be clearer to state "Multiple orgs with the same name found". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. pkg/query-service/dao/sqlite/rbac.go:565
- Draft comment:
Typographical/lexicographical issue: In GetResetPasswordEntry, the error message "Multiple entries for reset token is found" is grammatically awkward. Consider rephrasing it to "Multiple entries for reset token were found" or "Found multiple entries for reset token" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_35EmVYSQfZsV5RlT
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
| @@ -281,7 +281,7 @@ function Members(): JSX.Element { | |||
| const { joinedOn } = record; | |||
| return ( | |||
| <Typography> | |||
| {dayjs.unix(Number(joinedOn)).format(DATE_TIME_FORMATS.MONTH_DATE_FULL)} | |||
| {dayjs(joinedOn).format(DATE_TIME_FORMATS.MONTH_DATE_FULL)} | |||
There was a problem hiding this comment.
The joinedOn field appears to contain Unix timestamps in seconds (e.g., 1739884129).
Using dayjs() directly will interpret this as milliseconds, resulting in incorrect dates.
There was a problem hiding this comment.
so I have updated the code to return time instead of unix timestamp. Those are the backend changes.
There was a problem hiding this comment.
@ahmadshaheer do you remember how you did it for multiple ingestion keys? You added a case for handling zero values of time as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/SigNoz/platform-pod/issues/459
Note: This PR doesn't cover anything related to creating multiple ORG. that will be tackled in the end.
Also org id is not enforced on groups as right now groups are initalized during startup we will have to solve for it differently
Important
Enhances multitenancy by removing user flags, updating Appdex settings, and refactoring models and database schema for better organization support.
org_idfrom context; migration creates new table and drops old one.org,users, andinvites.typespackage forUser,Invite,Organization,Group, andApdexSettings.org_idinapdex_settingsandgroups.created_by,updated_at,updated_bytoorganizations,users,groups.created_atto timestamp and boolean fields to boolean type.setFlags.tsand related frontend components.This description was created by
for f991e73. It will automatically update as commits are pushed.