Skip to content

Feature implementation from commits ab2d6a4..9e84b8d#5

Open
yashuatla wants to merge 15 commits intofeature-base-5from
feature-head-5
Open

Feature implementation from commits ab2d6a4..9e84b8d#5
yashuatla wants to merge 15 commits intofeature-base-5from
feature-head-5

Conversation

@yashuatla
Copy link
Copy Markdown
Owner

This PR contains changes from a range of commits from the original repository.

Commit Range: ab2d6a4..9e84b8d
Files Changed: 77 (49 programming files)
Programming Ratio: 63.6%

Commits included:

giograno and others added 15 commits April 11, 2025 14:24
)

Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>
…e docker-base-images group (localstack#12523)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…alstack#12500)

Prior to this commit, --profile must be specified as a top-level option, e.g. localstack --profile test start.  Now it can be specified at any point, e.g. localstack start --profile test.
Co-authored-by: LocalStack Bot <localstack-bot@users.noreply.github.com>
Co-authored-by: Alexander Rashed <alexander.rashed@localstack.cloud>
else:
raise NotImplementedError

if not stack:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Potential null reference exception.

The code checks if stack is None after it has already been dereferenced in previous code paths, which could cause a runtime exception.

Current Code (Diff):

        if not stack:
            # aws will silently ignore invalid stack names - we should do the same
            return
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
if not stack:
# This check should be moved before any usage of stack variable
# or the variable should be properly initialized

# aws will silently ignore invalid stack names - we should do the same
return

# TODO: actually delete
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Incomplete resource deletion.

The DeleteStack implementation only sets the stack status to DELETE_COMPLETE without actually deleting resources, which could lead to orphaned resources.

Current Code (Diff):

        # TODO: actually delete
        stack.set_stack_status(StackStatus.DELETE_COMPLETE)
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
# TODO: actually delete
# Implement actual resource deletion before setting status
# For example: delete_stack_resources(stack)
stack.set_stack_status(StackStatus.DELETE_COMPLETE)

return

self._proxy_capture_input_event(event_formatted)
trace_header = context.trace_context["aws_trace_header"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Unsafe dictionary access may cause KeyError.

Direct access to context.trace_context["aws_trace_header"] will cause a KeyError if the key doesn't exist, crashing the application during event processing.

Current Code (Diff):

-         trace_header = context.trace_context["aws_trace_header"]
+         trace_header = context.trace_context.get("aws_trace_header", None)
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
trace_header = context.trace_context["aws_trace_header"]
trace_header = context.trace_context.get("aws_trace_header", None)

job["MediaFormat"] = SUPPORTED_FORMAT_NAMES[format]
duration = ffprobe_output["format"]["duration"]

if float(duration) >= MAX_AUDIO_DURATION_SECONDS:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Missing validation before float conversion.

Converting duration to float without validation could cause crashes if duration is not a valid number.

Current Code (Diff):

-             if float(duration) >= MAX_AUDIO_DURATION_SECONDS:
+             if duration and isinstance(duration, (str, int, float)) and float(duration) >= MAX_AUDIO_DURATION_SECONDS:
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
if float(duration) >= MAX_AUDIO_DURATION_SECONDS:
if duration and isinstance(duration, (str, int, float)) and float(duration) >= MAX_AUDIO_DURATION_SECONDS:

duration = ffprobe_output["format"]["duration"]

if float(duration) >= MAX_AUDIO_DURATION_SECONDS:
failure_reason = "Invalid file size: file size too large. Maximum audio duration is 4.000000 hours.Check the length of the file and try your request again."
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness Issue

Hardcoded duration value in error message.

Error message hardcodes '4.000000 hours' instead of using the MAX_AUDIO_DURATION_SECONDS constant, causing inconsistency if the constant changes.

Current Code (Diff):

-                 failure_reason = "Invalid file size: file size too large. Maximum audio duration is 4.000000 hours.Check the length of the file and try your request again."
+                 failure_reason = f"Invalid file size: file size too large. Maximum audio duration is {MAX_AUDIO_DURATION_SECONDS/3600:.6f} hours.Check the length of the file and try your request again."
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
failure_reason = "Invalid file size: file size too large. Maximum audio duration is 4.000000 hours.Check the length of the file and try your request again."
failure_reason = f"Invalid file size: file size too large. Maximum audio duration is {MAX_AUDIO_DURATION_SECONDS/3600:.6f} hours.Check the length of the file and try your request again."

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.