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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow file uploads with attribute #404

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions spec/attribute_spec.cr
Expand Up @@ -19,6 +19,11 @@ describe "Avram::Attribute" do
empty_array = Avram::Attribute.new(name: :empty_array, param: nil, value: [] of String, param_key: "test_form")
empty_array.value.should_not be_nil
end

it "returns nil for empty uploads" do
empty_upload = Avram::Attribute.new(name: :empty_upload, param: nil, value: UploadedFile.new(""), param_key: "test_form")
empty_upload.value.should be_nil
end
end

describe "#original_value" do
Expand All @@ -31,6 +36,11 @@ describe "Avram::Attribute" do
empty_array = Avram::Attribute.new(name: :empty_array, param: nil, value: [] of String, param_key: "test_form")
empty_array.original_value.should_not be_nil
end

it "returns nil for empty uploads" do
empty_upload = Avram::Attribute.new(name: :empty_upload, param: nil, value: UploadedFile.new(""), param_key: "test_form")
empty_upload.original_value.should be_nil
end
end

it "can reset errors" do
Expand Down
38 changes: 38 additions & 0 deletions spec/define_attribute_spec.cr
Expand Up @@ -5,6 +5,8 @@ private class Operation < Post::SaveOperation
attribute terms_of_service : Bool
attribute best_kind_of_bear : String = "black bear"
attribute default_is_false : Bool = false
attribute thumb : UploadedFile

before_save prepare

def prepare
Expand All @@ -25,6 +27,7 @@ describe "attribute in operations" do

it "generates a list of attributes" do
operation.attributes.map(&.name).should eq [
:thumb,
:default_is_false,
:best_kind_of_bear,
:terms_of_service,
Expand Down Expand Up @@ -107,6 +110,41 @@ describe "attribute in operations" do
end
end

describe "file_attribute in operation" do
it "is a PermittedAttribute" do
operation.thumb.should be_a(Avram::PermittedAttribute(Avram::Uploadable?))
operation.thumb.name.should eq(:thumb)
operation.thumb.param_key.should eq("post")
end

it "is included in the list of attributes" do
operation.attributes.map(&.name).should contain(:thumb)
end

it "gracefully handles invalid params" do
operation = operation({"thumb" => "not a file"})
operation.thumb.value.should be_nil
operation.thumb.errors.first.should eq "is invalid"
end

it "includes file attribute errors when calling SaveOperation#valid?" do
operation = operation({"thumb" => "not a file"})
operation.setup_required_database_columns
operation.valid?.should be_false
end

it "can still save to the database" do
params = {"thumb" => UploadedFile.new("thumb.png")}
operation = upload_operation(params)
operation.setup_required_database_columns
operation.save.should eq true
end
end

private def operation(attrs = {} of String => String)
Operation.new(Avram::Params.new(attrs))
end

private def upload_operation(attrs = {} of String => Avram::Uploadable)
Operation.new(UploadParams.new(attrs))
end
8 changes: 8 additions & 0 deletions spec/nested_save_operation_spec.cr
Expand Up @@ -47,6 +47,14 @@ private class NestedParams
def get(key)
raise "Not implemented"
end

def nested_file?(key)
raise "Not implemented"
end

def nested_file(key)
raise "Not implemented"
end
end

describe "Avram::SaveOperation with nested operation" do
Expand Down
16 changes: 16 additions & 0 deletions spec/support/upload_params.cr
@@ -0,0 +1,16 @@
class UploadParams < Avram::Params
include Avram::Paramable

@uploads : Hash(String, UploadedFile) = {} of String => UploadedFile

def initialize(@uploads)
end

def nested_file?(key) : Hash(String, UploadedFile)
@uploads
end

def nested_file(key) : Hash(String, UploadedFile)
@uploads
end
end
9 changes: 9 additions & 0 deletions spec/support/uploaded_file.cr
@@ -0,0 +1,9 @@
class UploadedFile
include Avram::Uploadable

def initialize(filename : String)
@name = "part.name"
@tempfile = File.tempfile(@name) { |file| File.write(file.path, "tmp") }
@metadata = HTTP::FormData::FileMetadata.new(filename)
end
end
9 changes: 7 additions & 2 deletions src/avram/attribute.cr
Expand Up @@ -5,7 +5,12 @@ class Avram::Attribute(T)
getter :param_key
@errors = [] of String

def initialize(@name : Symbol, @param : String?, @value : T, @param_key : String)
def initialize(
@name : Symbol,
@param : Avram::Uploadable | String?,
@value : T,
@param_key : String
)
@original_value = @value
end

Expand Down Expand Up @@ -55,7 +60,7 @@ class Avram::Attribute(T)
end

private def ensure_no_blank(value : T)
if value.is_a?(String) && value.blank?
if value.is_a?(Avram::Uploadable | String) && value.blank?
nil
else
value
Expand Down
39 changes: 33 additions & 6 deletions src/avram/define_attribute.cr
Expand Up @@ -49,15 +49,23 @@ module Avram::DefineAttribute

{% ATTRIBUTES << type_declaration %}

{% type = type_declaration.type %}
{% name = type_declaration.var %}
{%
default_value = if type_declaration.value.is_a?(Nop)
target_type = type_declaration.type

includers = Avram::Uploadable.includers
includers.each { |includer| includers += includer.all_subclasses }
is_uploadable = includers.includes?(target_type.resolve)

type = is_uploadable ? Avram::Uploadable : target_type
name = type_declaration.var

default_value = if type_declaration.value.is_a?(Nop) || is_uploadable
nil
else
type_declaration.value
end
%}

@_{{ name }} : Avram::Attribute({{ type }}?)?

ensure_base_attributes_method_is_present
Expand All @@ -84,17 +92,36 @@ module Avram::DefineAttribute
end

private def {{ name }}_param
params.nested(self.class.param_key)["{{ name }}"]?
{% if is_uploadable %}
if file = params.nested_file?(self.class.param_key)
file["{{ name }}"]?
end
{% else %}
params.nested(self.class.param_key)["{{ name }}"]?
{% end %}
end

private def {{ name }}_param_given?
params.nested(self.class.param_key).has_key?("{{ name }}")
{% if is_uploadable %}
file = params.nested_file?(self.class.param_key)
file && file.has_key?("{{ name }}")
{% else %}
params.nested(self.class.param_key).has_key?("{{ name }}")
{% end %}
end

def set_{{ name }}_from_param(attribute : Avram::Attribute)
parse_result = {{ type }}::Lucky.parse({{ name }}_param)
if parse_result.is_a? Avram::Type::SuccessfulCast
attribute.value = parse_result.value
{% if is_uploadable %}
attribute.value = if parse_result.value.class == {{ target_type }}
parse_result.value
else
{{ target_type }}.new(parse_result.value)
end
{% else %}
attribute.value = parse_result.value
{% end %}
else
attribute.add_error "is invalid"
end
Expand Down
8 changes: 8 additions & 0 deletions src/avram/params.cr
Expand Up @@ -30,4 +30,12 @@ class Avram::Params
def get(key)
@hash[key]
end

def nested_file?(key) : Hash(String, String)
@hash
end

def nested_file(key) : Hash(String, String)
@hash
end
end
54 changes: 54 additions & 0 deletions src/avram/uploadable.cr
@@ -0,0 +1,54 @@
# Include this module in classes that represent an uploaded file from a form
module Avram::Uploadable
getter name : String
getter tempfile : File
getter metadata : HTTP::FormData::FileMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it might be possible to abstract this metadata somewhat?

I love your approach of being able to use this module outside of Lucky::UploadedFile, but this means it could be used for files that arrived some other way, not necessarily via HTTP. These could of course simply construct their own HTTP::FormData::FileMetadata, but it would feel a bit awkward imho.

Also, avram is supposed to be a general purpose ORM. Seeing things related to HTTP here is at least a little bit confusing.

Maybe it might suffice to simply add the attributes from HTTP::FormData::FileMetadata that are relevant here directly (possibly size and the different timestamps). Personally I would be fine with removing this altogether. At this point we already have the complete file to determine the size reliably and I have never looked at what timestamps the browser sends with files before.

Copy link
Contributor Author

@wout wout Jun 25, 2020

Choose a reason for hiding this comment

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

Very good point! We only need it for the original filename. I think the metadata property can move back to Lucky::UploadedFile and Avram::Uploadable can declare an interface for filename:

module Avram::Uploadable
  abstract filename : String
  # ...
end

So that includers can implement it however they like:

module Lucky::UploadedFile
  getter metadata : HTTP::FormData::FileMetadata
  # ...
  def filename : String
    metadata.filename.to_s
  end
end

I think we'll also need to rethink the name property as well (as discussed in luckyframework/lucky#1147). I can't seem to find a place where it's actually used. For clarity, at least we could change it to partname, and raise a compile-time error on the name method referencing both filename and partname.


macro included
# :nodoc:
def initialize(file : Avram::Uploadable)
@name = file.name
@tempfile = file.tempfile
@metadata = file.metadata
end
end

# Returns the path of the File as a String
#
# ```
# uploaded_file_object.path # => String
# ```
def path : String
@tempfile.path
end

# Returns the original file name as a String
#
# ```
# uploaded_file_object.filename # => String
# ```
def filename : String
metadata.filename.to_s
end

# If no file was selected in the form's file input, this will return `true`
#
# ```
# uploaded_file_object.blank? # => Bool
# ```
def blank? : Bool
filename.blank?
end

module Lucky
include Avram::Type

def parse(value : Avram::Uploadable)
SuccessfulCast(Avram::Uploadable).new(value)
end

def parse(value : String?)
FailedCast.new
end
end
end