Replace String with Ulids to store ids #1345
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
chinmaym07
force-pushed
the
rep-string-ulid-1125
branch
from
02f8a12 to
eae9f5e
Compare
chinmaym07
force-pushed
the
rep-string-ulid-1125
branch
from
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🔭 Outside diff range comments (9)
src/handlers/http/rbac.rs (1)
54-57:⚠️ Potential issueType mismatch: assigning
Stringto anUlid
user.username()returns a&str, but theidfield 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,
ulidis not a type – useUlid.default_value_t = String::from("parseable-connect")no longer matches the new type and will not compile.clapdoesn’t deriveValueParserforUlid; you need a customvalue_parser(or keep this field asStringand 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_idnowUlid→ downstream breakage.
ConsumerConfig::validatestill callsself.group_id.is_empty()(line 616) andapply_to_configdoesset("group.id", &self.group_id). Both APIs expect a&str, not anUlid. Convert to string (or revert toString) and drop theis_emptycheck.- 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
Permisssionstruct – multiple type errors and logic bug.pub id: ulid, ... pub fn new(id: ulid) -> Self { Self { id: id.clone(), group: id,Issues:
ulid→Ulid.groupisStringbut you assign anUlid.- Calling
.clone()onUlidalreadyCopy(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 anUlidbutSenderPipe.idis stillString(type Id = String).
Subsequent.retain(|x| x.id != self.id)comparesStringwithUlid, 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 asStringand revert the function signatures).Also applies to: 83-90
108-125: 🛠️ Refactor suggestion
channelreturns mismatched types & redundant clone.
SenderPipe.idexpectsId(String) but receivesUlid, then.clone()is called. Remove the clone and align types per previous comment.Additionally change:
- id: id.clone(), + id,once the
Idalias is fixed.src/parseable/streams.rs (1)
104-118:⚠️ Potential issueLower-case
ulidand optional default collide with new type.pub ingestor_id: Option<ulid>, ... ingestor_id: Option<ulid>,
- Use
Ulidnotulid.- Tests/instantiation still pass
Noneor 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_createparam 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
discoverexpectsclient_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 withUlidproduces wrong filename.
hostname.push_str(id);
push_strexpects&str, butidisOption<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()); }
chinmaym07
force-pushed
the
rep-string-ulid-1125
branch
3 times, most recently
from
cb74dd7 to
b617229
Compare
chinmaym07
force-pushed
the
rep-string-ulid-1125
branch
from
b617229 to
e2d4ae2
Compare
Fixes #1125 .
Description
This PR has:
Summary by CodeRabbit