Skip to content

fix(report): raise FossologyApiError when report ID cannot be parsed#178

Open
Valyrian-Code wants to merge 2 commits into
fossology:mainfrom
Valyrian-Code:fix/generate-report-parse-id
Open

fix(report): raise FossologyApiError when report ID cannot be parsed#178
Valyrian-Code wants to merge 2 commits into
fossology:mainfrom
Valyrian-Code:fix/generate-report-parse-id

Conversation

@Valyrian-Code
Copy link
Copy Markdown

Summary

Closes #176.

In fossology/report.py:56, the regex used to extract the report ID from the API response message was:

report_id = re.search("[0-9]*$", response.json()["message"])
return report_id[0]

[0-9]* (zero or more) always matches — even on a string with no trailing digits — so re.search returns a match object whose group 0 is an empty string "". The caller received that empty string with no error raised, and the failure surfaced later as a confusing 404 or type error inside download_report.

Change

  • Switch the quantifier to [0-9]+$ (one or more)
  • Raise FossologyApiError if no ID can be parsed, with a descriptive message that includes the original response text
  • Drop the # type: ignore since match is now narrowed to non-None before use

Compatibility

Happy-path behaviour is identical: a message like "Report will be generated in the back ground, report id is 42" still parses to "42". The only behaviour change is that callers now get a clear exception instead of a silent empty string when the message format is unexpected.

Test plan

  • New regression test test_generate_report_unparseable_message mocks a 201 response with no trailing digits and asserts FossologyApiError is raised
  • pytest tests/test_report.py::test_generate_report_unparseable_message passes locally
  • ruff check clean
  • CI green

Open questions for maintainers

  1. Return type: the function's docstring says :rtype: int but the function returns str. I kept it as str to minimise the diff. Happy to coerce to int and update the docstring in a follow-up if you'd prefer.
  2. Error class: I raised FossologyApiError since a real response is in hand. Let me know if a dedicated parsing exception would fit better.

The regex used to extract the report ID from the API response message
was `[0-9]*$`. With `*` (zero or more), the regex always matches even
on a string with no trailing digits — `re.search` returns a match
object whose group 0 is an empty string. The caller received `""` and
no error, which then failed downstream as a confusing 404 or type
error in `download_report`.

Switch the quantifier to `+` (one or more) and raise `FossologyApiError`
when no ID can be parsed, so the failure surfaces at the right point
with a clear message.

Adds a regression test that mocks a 201 response whose message has no
trailing digits and asserts the new error path.

Closes fossology#176

Signed-off-by: RAJVEER42 <irajveer.bishnoi2310@gmail.com>
Copilot AI review requested due to automatic review settings May 18, 2026 16:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves generate_report error handling by making report ID parsing fail clearly when the FOSSology API response message does not end with a numeric ID.

Changes:

  • Requires at least one trailing digit when extracting the report ID.
  • Raises FossologyApiError when parsing fails.
  • Adds a regression test for an unparseable report-generation response message.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
fossology/report.py Updates report ID parsing and adds explicit parse-failure handling.
tests/test_report.py Adds regression coverage for a 201 report response without a parseable report ID.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread fossology/report.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@Valyrian-Code
Copy link
Copy Markdown
Author

Valyrian-Code commented May 21, 2026

Hi @deveaud-m ,

I’ve provided a minimal patch for this PR. Could you please review it when you have a chance?

@Valyrian-Code
Copy link
Copy Markdown
Author

Valyrian-Code commented May 22, 2026

Hi @deveaud-m

Heads up: the failing Integration Tests job here isn't caused by this PR. The pytest run itself completes successfully (192 passed, 2 skipped, 5 xfailed, 2 xpassed); the failure is in the "upload codecoverage results only if we are on the repository fossology/fossology-python" step that runs afterwards:

poetry run codecov -t 
codecov: error: argument --token/-t: expected one argument
##[error]Process completed with exit code 2.

The step's -t ${{ secrets.CODECOV_TOKEN }} substitutes to an empty value on fork PRs (secrets aren't passed to fork workflows), and the codecov CLI then exits non-zero, which fails the whole job. The step's intent appears to be "skip on forks" per its name, but the current conditional doesn't actually prevent it from running.

Happy to leave this for you to handle on the workflow side — just flagging so you don't have to dig.

Comment thread fossology/report.py
return report_id[0] # type: ignore
try:
message = response.json()["message"]
except (ValueError, KeyError, TypeError) as exc:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a test available to cover this except path?

If not please create one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generate_report silently returns empty string when report ID cannot be parsed

3 participants