-
-
Notifications
You must be signed in to change notification settings - Fork 142
Replace String with Ulids to store ids #1345
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
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
02f8a12
to
eae9f5e
Compare
eae9f5e
to
5daf562
Compare
""" WalkthroughThe changes systematically replace the use of Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Module
participant Ulid
Caller->>Module: Call method with id: Ulid
Module->>Ulid: Validate/Generate/Use Ulid
Ulid-->>Module: Return Ulid value
Module-->>Caller: Processed result with Ulid id
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (14)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 23
🔭 Outside diff range comments (9)
src/handlers/http/rbac.rs (1)
54-57
:⚠️ Potential issueType mismatch: assigning
String
to anUlid
user.username()
returns a&str
, but theid
field is nowUlid
; this will not compile. Decide whether
a) the field should remain aString
, or
b) you must convert the username to aUlid
(likely wrong conceptually).If you really need a ULID:
- id: user.username().to_owned(), + id: Ulid::from_string(user.username()) + .expect("username must be a valid ULID"),…but double-check the domain model—usernames and IDs are usually distinct.
src/users/filters.rs (1)
91-95
:⚠️ Potential issueWrong type for
Rules.id
- pub id: ulid, + pub id: Ulid,src/connectors/kafka/config.rs (2)
21-30
:⚠️ Potential issueType mismatch & invalid default for
client_id
.
pub client_id: ulid,
ulid
is not a type – useUlid
.default_value_t = String::from("parseable-connect")
no longer matches the new type and will not compile.clap
doesn’t deriveValueParser
forUlid
; you need a customvalue_parser
(or keep this field asString
and convert later).Minimal compile-fix:
- #[arg( - long = "client-id", - env = "P_KAFKA_CLIENT_ID", - required = false, - default_value_t = String::from("parseable-connect"), - value_name = "client_id", - help = "Client ID for Kafka connection" - )] - pub client_id: ulid, + #[arg(long = "client-id", env = "P_KAFKA_CLIENT_ID", value_parser = parse_ulid)] + pub client_id: Ulid,and add
fn parse_ulid(s: &str) -> Result<Ulid, String> { Ulid::from_string(s).map_err(|e| e.to_string()) }(or revert to
String
).
Similar corrections are needed for every field switched to ULID.
72-85
:⚠️ Potential issue
group_id
nowUlid
→ downstream breakage.
ConsumerConfig::validate
still callsself.group_id.is_empty()
(line 616) andapply_to_config
doesset("group.id", &self.group_id)
. Both APIs expect a&str
, not anUlid
. Convert to string (or revert toString
) and drop theis_empty
check.- pub group_id: ulid, + pub group_id: Ulid, ... - if self.group_id.is_empty() { + if self.group_id == Ulid::nil() { // or custom check / removeand in
apply_to_config
- .set("group.id", &self.group_id) + .set("group.id", &self.group_id.to_string())Also applies to: 80-83
src/storage/mod.rs (1)
187-201
:⚠️ Potential issue
Permisssion
struct – multiple type errors and logic bug.pub id: ulid, ... pub fn new(id: ulid) -> Self { Self { id: id.clone(), group: id,Issues:
ulid
→Ulid
.group
isString
but you assign anUlid
.- Calling
.clone()
onUlid
alreadyCopy
(unnecessary).Suggested patch:
- pub id: ulid, + pub id: Ulid, ... - pub fn new(id: ulid) -> Self { - Self { - id: id.clone(), - group: id, + pub fn new(id: Ulid) -> Self { + Self { + id, + group: id.to_string(),src/livetail.rs (3)
29-33
:⚠️ Potential issueImport typo blocks compilation.
use ulid: Ulid;should be
use ulid::Ulid;
35-50
:⚠️ Potential issueType inconsistency between pipes and registry.
new_pipe(id: ulid, …)
passes anUlid
butSenderPipe.id
is stillString
(type Id = String
).
Subsequent.retain(|x| x.id != self.id)
comparesString
withUlid
, causing compile errors and logic flaws.Decide on a single type:
-type Id = String; +type Id = Ulid;Update
SenderPipe
,ReceiverPipe
, registry map key, and all usages toUlid
(or keep everything asString
and revert the function signatures).Also applies to: 83-90
108-125
: 🛠️ Refactor suggestion
channel
returns mismatched types & redundant clone.
SenderPipe.id
expectsId
(String
) but receivesUlid
, then.clone()
is called. Remove the clone and align types per previous comment.Additionally change:
- id: id.clone(), + id,once the
Id
alias is fixed.src/parseable/streams.rs (1)
104-118
:⚠️ Potential issueLower-case
ulid
and optional default collide with new type.pub ingestor_id: Option<ulid>, ... ingestor_id: Option<ulid>,
- Use
Ulid
notulid
.- Tests/instantiation still pass
None
or strings; ensure callers supplyOption<Ulid>
.- pub ingestor_id: Option<ulid>, + pub ingestor_id: Option<Ulid>, ... - ingestor_id: Option<ulid>, + ingestor_id: Option<Ulid>,
♻️ Duplicate comments (2)
src/connectors/kafka/config.rs (1)
105-113
:group_instance_id
– same issues as above.Lower-case
ulid
, invalid default value string, and missing conversion when passed to rdkafka (set("group.instance.id", …)
). Fix identically togroup_id
.src/parseable/streams.rs (1)
768-769
:Streams::get_or_create
param type needs update.
ingestor_id: Option<ulid>
→Option<Ulid>
to stay consistent withStream::new
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/cli.rs
(2 hunks)src/connectors/kafka/config.rs
(4 hunks)src/correlation.rs
(2 hunks)src/handlers/http/kinesis.rs
(1 hunks)src/handlers/http/modal/mod.rs
(3 hunks)src/handlers/http/rbac.rs
(2 hunks)src/livetail.rs
(3 hunks)src/oidc.rs
(2 hunks)src/parseable/streams.rs
(4 hunks)src/prism/home/mod.rs
(2 hunks)src/rbac/mod.rs
(2 hunks)src/rbac/user.rs
(2 hunks)src/storage/mod.rs
(1 hunks)src/users/filters.rs
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/handlers/http/kinesis.rs (1)
src/utils/json/flatten.rs (3)
flatten
(58-93)generic_flattening
(269-328)has_more_than_max_allowed_levels
(335-348)
🔇 Additional comments (3)
src/oidc.rs (1)
64-66
: API expectation mismatch foropenid::Client::discover
discover
expectsclient_id: String
, but you’re now passing anUlid
.
Convert to a string or keep the original type.- DiscoveredClient::discover(self.id, self.secret, redirect_uri.to_string(), self.issuer) + DiscoveredClient::discover(self.id.to_string(), self.secret, redirect_uri.to_string(), self.issuer)src/parseable/streams.rs (2)
29-33
: Good: correct import ofUlid
.
use ulid::Ulid;
is syntactically correct.
173-178
: String concatenation withUlid
produces wrong filename.
hostname.push_str(id);
push_str
expects&str
, butid
isOption<Ulid>
. Convert properly:-if let Some(id) = &self.ingestor_id { - hostname.push_str(id); +if let Some(id) = &self.ingestor_id { + hostname.push_str(&id.to_string()); }
cb74dd7
to
b617229
Compare
b617229
to
e2d4ae2
Compare
Fixes #1125 .
Description
This PR has:
Summary by CodeRabbit