Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,7 @@ pub struct FetchEventsOptions<'a> {
/// Fields to include in the response
pub fields: &'a [&'a str],
/// Project ID to filter events by
pub project_id: &'a str,
pub project_id: Option<&'a str>,
/// Cursor for pagination
pub cursor: Option<&'a str>,
/// Query string to filter events
Expand All @@ -1480,8 +1480,14 @@ impl<'a> FetchEventsOptions<'a> {
params.push(format!("cursor={}", QueryArg(cursor)));
}

params.push(format!("project={}", QueryArg(self.project_id)));
params.push(format!("query={}", QueryArg(self.query)));
if let Some(project) = self.project_id {
if !project.is_empty() {
params.push(format!("project={}", QueryArg(project)));
}
}
if !self.query.is_empty() {
params.push(format!("query={}", QueryArg(self.query)));
}
params.push(format!("per_page={}", self.per_page));
params.push(format!("statsPeriod={}", QueryArg(self.stats_period)));
params.push(format!("sort={}", QueryArg(self.sort)));
Expand Down
63 changes: 54 additions & 9 deletions src/commands/logs/list.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use anyhow::Result;
use clap::Args;

Expand All @@ -20,6 +22,11 @@ fn validate_max_rows(s: &str) -> Result<usize> {
}
}

/// Check if a project identifier is numeric (project ID) or string (project slug)
fn is_numeric_project_id(project: &str) -> bool {
!project.is_empty() && project.chars().all(|c| c.is_ascii_digit())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Numeric ID Check Fails for Digit Slugs

The is_numeric_project_id function incorrectly returns true for project slugs that consist entirely of digits, causing them to be passed as a project_id parameter instead of being included in the query string. This can lead to failed API requests or incorrect results. Additionally, the function incorrectly returns true for empty strings, which results in an empty project argument being treated as a numeric ID, causing no project filter to be applied and logs from all organization projects to be returned.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may be a valid concern, at least the part about an empty project. We should validate that users cannot provide an empty string for this argument

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍


/// Fields to fetch from the logs API
const LOG_FIELDS: &[&str] = &[
"sentry.item_id",
Expand All @@ -37,7 +44,7 @@ pub(super) struct ListLogsArgs {
org: Option<String>,

#[arg(short = 'p', long = "project")]
#[arg(help = "The project ID (slug not supported).")]
#[arg(help = "The project ID or slug.")]
project: Option<String>,

#[arg(long = "max-rows", default_value = "100")]
Expand Down Expand Up @@ -70,29 +77,36 @@ pub(super) fn execute(args: ListLogsArgs) -> Result<()> {

let api = Api::current();

let query = if args.query.is_empty() {
None
// Pass numeric project IDs as project parameter, otherwise pass as query string -
// current API does not support project slugs as a parameter.
let (query, project_id) = if is_numeric_project_id(project) {
(Cow::Borrowed(&args.query), Some(project.as_str()))
} else {
Some(args.query.as_str())
let query = if args.query.is_empty() {
format!("project:{project}")
} else {
format!("project:{project} {}", args.query)
};
(Cow::Owned(query), None)
Comment thread
shellmayr marked this conversation as resolved.
Comment thread
shellmayr marked this conversation as resolved.
};

execute_single_fetch(&api, org, project, query, LOG_FIELDS, &args)
execute_single_fetch(&api, org, project_id, &query, LOG_FIELDS, &args)
}

fn execute_single_fetch(
api: &Api,
org: &str,
project: &str,
query: Option<&str>,
project_id: Option<&str>,
query: &str,
fields: &[&str],
args: &ListLogsArgs,
) -> Result<()> {
let options = FetchEventsOptions {
dataset: Dataset::Logs,
fields,
project_id: project,
project_id,
cursor: None,
query: query.unwrap_or(""),
query,
per_page: args.max_rows,
stats_period: "90d",
sort: "-timestamp",
Expand Down Expand Up @@ -128,3 +142,34 @@ fn execute_single_fetch(

Ok(())
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_is_numeric_project_id_purely_numeric() {
assert!(is_numeric_project_id("123456"));
assert!(is_numeric_project_id("1"));
assert!(is_numeric_project_id("999999999"));
}

#[test]
fn test_is_numeric_project_id_alphanumeric() {
assert!(!is_numeric_project_id("abc123"));
assert!(!is_numeric_project_id("123abc"));
assert!(!is_numeric_project_id("my-project"));
}

#[test]
fn test_is_numeric_project_id_numeric_with_dash() {
assert!(!is_numeric_project_id("123-45"));
assert!(!is_numeric_project_id("1-2-3"));
assert!(!is_numeric_project_id("999-888"));
}

#[test]
fn test_is_numeric_project_id_empty_string() {
assert!(!is_numeric_project_id(""));
}
}
2 changes: 1 addition & 1 deletion tests/integration/_cases/logs/logs-list-help.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Options:
in key:value format.

-p, --project <PROJECT>
The project ID (slug not supported).
The project ID or slug.

--auth-token <AUTH_TOKEN>
Use the given Sentry auth token.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```
$ sentry-cli logs list --org wat-org --project 12345
? success
[BETA] The "logs" command is in beta. The command is subject to breaking changes, including removal, in any Sentry CLI release.
No logs found

```
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
```
$ sentry-cli logs list --org wat-org --project 12345
$ sentry-cli logs list --org wat-org --project myproject
? success
[BETA] The "logs" command is in beta. The command is subject to breaking changes, including removal, in any Sentry CLI release.
No logs found
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
```
$ sentry-cli logs list --project 12345
? success
[BETA] The "logs" command is in beta. The command is subject to breaking changes, including removal, in any Sentry CLI release.
+------------------+---------------------------+----------+--------------------------+----------------------+
| Item ID | Timestamp | Severity | Message | Trace |
+------------------+---------------------------+----------+--------------------------+----------------------+
| test-item-id-001 | 2025-01-15T10:30:00+00:00 | info | test_log_message_001 | test-trace-id-abc123 |
| test-item-id-002 | 2025-01-15T10:31:00+00:00 | error | test_error_message_002 | test-trace-id-def456 |
| test-item-id-003 | 2025-01-15T10:32:00+00:00 | warning | test_warning_message_003 | test-trace-id-ghi789 |
+------------------+---------------------------+----------+--------------------------+----------------------+

```
2 changes: 1 addition & 1 deletion tests/integration/_cases/logs/logs-list-with-data.trycmd
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
```
$ sentry-cli logs list
$ sentry-cli logs list --org wat-org --project myproject
? success
[BETA] The "logs" command is in beta. The command is subject to breaking changes, including removal, in any Sentry CLI release.
+------------------+---------------------------+----------+--------------------------+----------------------+
Expand Down
36 changes: 32 additions & 4 deletions tests/integration/logs.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::integration::{MockEndpointBuilder, TestManager};

#[test]
fn command_logs_with_api_calls() {
fn command_logs_with_api_calls_project_slug() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new(
"GET",
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&project=wat-project&query=&per_page=100&statsPeriod=90d&sort=-timestamp"
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&query=project:myproject&per_page=100&statsPeriod=90d&sort=-timestamp"
)
.with_response_file("logs/get-logs.json"),
)
Expand All @@ -15,19 +15,47 @@ fn command_logs_with_api_calls() {
}

#[test]
fn command_logs_no_logs_found() {
fn command_logs_with_api_calls_project_id() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new(
"GET",
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&project=12345&query=&per_page=100&statsPeriod=90d&sort=-timestamp"
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&project=12345&per_page=100&statsPeriod=90d&sort=-timestamp"
)
.with_response_file("logs/get-logs.json"),
)
.register_trycmd_test("logs/logs-list-with-data-project-id.trycmd")
.with_default_token();
}

#[test]
fn command_logs_no_logs_found_project_slug() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new(
"GET",
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&query=project:myproject&per_page=100&statsPeriod=90d&sort=-timestamp"
)
.with_response_body(r#"{"data": []}"#),
)
.register_trycmd_test("logs/logs-list-no-logs-found.trycmd")
.with_default_token();
}

#[test]
fn command_logs_no_logs_found_project_id() {
TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new(
"GET",
"/api/0/organizations/wat-org/events/?dataset=logs&field=sentry.item_id&field=trace&field=severity&field=timestamp&field=message&project=12345&per_page=100&statsPeriod=90d&sort=-timestamp"
)
.with_response_body(r#"{"data": []}"#),
)
.register_trycmd_test("logs/logs-list-no-logs-found-project-id.trycmd")
.with_default_token();
}

#[test]
fn command_logs_zero_max_rows() {
TestManager::new().register_trycmd_test("logs/logs-list-with-zero-max-rows.trycmd");
Expand Down