Skip to content
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

#4, refactor: add Rack env builder #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

requiemformemories
Copy link

Description

The Rack env build method is too large that hard to extend new behaviors. Add Hanami::Lambda::Env to improve it.

Related Issues

@requiemformemories requiemformemories force-pushed the refactor/add-rack-env-to-builder branch 2 times, most recently from f8c89e7 to 0979521 Compare June 25, 2024 13:52
@elct9620
Copy link
Owner

@requiemformemories please rebase main branch and sign your commit

Copy link
Owner

Choose a reason for hiding this comment

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

Please specify the correct types.

@requiemformemories
Copy link
Author

Thanks for the review. I made up for the RBS types and I also used another way to transform the headers.

::Hanami::Lambda::LAMBDA_EVENT => event,
::Hanami::Lambda::LAMBDA_CONTEXT => context
}.tap do |env|
content_type = headers.delete("Content-Type") ||
Copy link
Owner

Choose a reason for hiding this comment

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

To get "content-type" with different keys because all values are possibly received from Lambda event. We should not change it.

#
# @return [Hash] the hash of Rack environment
#
def to_h: () -> ::Hash[String, untyped]
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to use "allowlist" to list all possible hash value instead "untyped"

@requiemformemories
Copy link
Author

requiemformemories commented Aug 9, 2024

Currently I am facing some difficulties with type checking.

sig/hanami/lambda/env.rbs:14:55: [error] Type `::Hash` is generic but used as a non generic type
│ Diagnostic ID: RBS::InvalidTypeApplication
│
└       def initialize: (event: Hash[::String, String? | Hash], headers: Hash[::String, ::String], context: Object) -> void
                                                         ~~~~

(1) @event is quite complicated. For example, we might have event['requestContext']['http']['method'] and if we want to list all types, we have to list all ::Hash inside the ::Hash

lib/hanami/lambda/env.rb:25:32: [error] Cannot pass a value of type `(::String | nil)` as an argument of type `(::String | ::Hash | ::StringIO | ::Hash[::String, (::String | ::Hash)] | ::Object)`
│   (::String | nil) <: (::String | ::Hash | ::StringIO | ::Hash[::String, (::String | ::Hash)] | ::Object)
│     nil <: (::String | ::Hash | ::StringIO | ::Hash[::String, (::String | ::Hash)] | ::Object)
│       nil <: ::String
│
│ Diagnostic ID: Ruby::ArgumentTypeMismatch
│
└           env["CONTENT_TYPE"] = @content_type if @content_type

(2) In the source code we have env["CONTENT_TYPE"] = @content_type if @content_type and the if clause could make sure that only string could be store in env["CONTENT_TYPE"] . I have to define the value of @event could be nil or I could not pass the validation.

lib/hanami/lambda/env.rb:21:45: [error] Cannot pass a value of type `(::String | ::Hash)` as an argument of type `::String`
│   (::String | ::Hash) <: ::String
│     ::Hash <: ::String
│       ::Object <: ::String
│         ::BasicObject <: ::String
│
│ Diagnostic ID: Ruby::ArgumentTypeMismatch
│
└           ::Rack::RACK_INPUT => StringIO.new(@event["body"] || ""),
                                               ~~~~~~~~~~~~~~~~~~~~

(3) StringIO.new(@event["body"] || "") would also violate the type checking because Steep do not know the detail of @event.

@elct9620
Copy link
Owner

elct9620 commented Aug 9, 2024

(1) @event is quite complicated.

RBS support alias means we can write something like

# Nested "event" key-value
type event = Hash[String, event]

def initialize: (event: event, ...) -> ...

(2) env["CONTENT_TYPE"] = @content_type if @content_type

I think you need to think about why @content_type will be nil?

The nil usually is not the correct state

(3) StringIO.new(@event["body"] || "")

If we give initialize(event: ...) and then assign it to @event = event the RBS should know it's type. If not, it may be a bug.

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.

Refactor to add Rack env builder
2 participants