luckyframework / avram Goto Github PK

As a user,
I am altering the table to add a belongs to some other table which is not nullable.
but when i do this.
add_belongs_to product : Product, on_delete: :cascade, default: "1"
compiler complaints that there is no such default

I would like to pass fill_existing_with parameter. what should i do?

Validate valid email, downcase, etc.

Similar to:

• https://github.com/luckyframework/lucky_record/blob/1c0757fa6686d6687259e5b8c299a61e1d4815de/spec/model_spec.cr#L7
• https://github.com/luckyframework/lucky_record/blob/b7658340546171e783527af643e43e6b0426dc98/spec/support/custom_email.cr#L1
• Validation should be added in the parse method and use FailedCast if it is not in a valid format.

Also requested in the Lucky repo: luckyframework/lucky#612. Not sure if this should be tackled along with or separate from some custom (de)serialization for attributes.

It would be convenient to support fill_existing_with for add_belongs_to. Otherwise we need to do something like this whenever we want to alter an existing table with data:

add_belongs_to thing : Thing?, references: :thing, on_delete: :cascade
execute "UPDATE stuff SET thing_id = 1;"
execute "ALTER TABLE stuff ALTER COLUMN thing_id SET NOT NULL;"

Continue of this conversation part: https://gitter.im/luckyframework/Lobby?at=5c093ec628907a3c7bd9ccf6

To disallow bad queries that don't have indexes during compile time check if a query field is used that does not contain an index. This is a bad query and should not allow to pass this state and a database index should be added.

I'm not a database expert but the feature should not export find_by_field functions or in the query builder where fields.

There should be an override method like where_no_index to support also queries that are not optimized.

#57 (comment)

When crystal-lang/crystal#7481 is fixed

It would be great if lucky_record could support prefixed KSUID as a primary_key_type.

Currently the only supported types are bare int and UUID.

A modern ID is short, optionally k-sortable and optionally prefixed with an abbreviated model id. Example (from Stripe): inv_SojvcbC89I7uo5

It would be great if lucky models could be configured to
generate IDs like the above by adding a line such
as this to the model file:

primary_key_type KSUID, prefix: 'inv'

Crystal only has a Time class. How should LuckyRecord handle dates?

One way this could be done is to create a LuckyRecord::Date class that can be used and sets the time to 00:00:00. It would be stored to the db as a time.

Alternatively it would save to the db as a date. I'll have to think that through a bit more though. I'm open to suggestions if anyone has some thoughts about this.

because both crystal and pg support it

If I create a form based off of a model, but don’t include all of the required fields in both the html form and the form object, all created will fail silently.

Perhaps logging failed validations would make this less confusing to developers?

Brought to my attention here: https://gitter.im/luckyframework/Lobby?at=5a5925eb97cedeb0482dec9e

"Form" may note be the best name because you can use "Forms" for things that are not ever saved with a traditional form. For example, you might use a LuckyRecord "Form" for saving something from a CSV. It's also confusing to talk about because it's unclear if you're talking about a LuckyRecord "Form" or an HTML form. JSON APIs also doesn't use "forms."

I'd love to come up with a better name. "Saver" or "Commiter" makes sense, but sounds super weird: UserSaver, etc.

Some other ideas are to come up with some other term that can take on its own meaning, similar to how Trailblazer has "Cells". It doesn't really mean anything in software so it can take on its own meaning. Feel free to post ideas here :)

Brainstorming:

• UserSaver
• UserCommiter
• UserForge
• UserMutation
• UserChangeset
• UserRecord
• UserTransaction
• UserOperation
• UserReactor
• SaveUser

It is a common pattern when using VirtualForms to yield the form object and one or more additional objects when the form is valid. For example:

class MyForm < Avram::VirtualForm
  virtual some_field : String

  def submit
    validate_required some_field

    if valid?
      # Only yield the user if form is valid
      yield self, @user
    else
      yield self, nil
    end
  end
end

# Used like this
MyForm.submit do |form, user|
  if user
    # Do something
  else 
    # Do something else
  end
end

This can get repetitive and error-prone.

Another real-life example is here:
https://github.com/luckyframework/lucky_cli/pull/312/files#diff-b6e4ba970ba3c90d012a7fded94d57b2

Proposed solution

From comment here: #69 (comment)

class SearchUsers < Avram::VirtualOperation
  attribute query : String
  step { validate_size query, min: 2 }
  
  def result
    if valid? # If no validation errors on attributes
      UserQuery.search(query.value)
    else
      UserQuery.new.none
    end
  end
end

SearchUsers.run(params) do |operation, results|
  # do something
end

An example of the RequestResetPassword operation. Original here (https://github.com/luckyframework/lucky_cli/blob/d78429c8e40f97af20437cb8199de51c4962159f/src/base_authentication_app_skeleton/src/operations/request_password_reset.cr#L1)

class RequestPasswordReset < Avram::VirtualOperation
  # You can modify this in src/operations/mixins/user_from_email.cr
  include UserFromEmail
  step validate_email

  attribute email : String

  def result
    user_from_email
  end

  def validate_email
    validate_required email
    if user_from_email?
      email.add_error "is not in our system"
    end
  end
end

Crystal version: 0.25.1
LuckyRecord version: 0.6.0

When two models A and B are defined in the project and model B is associated to A like so:

table :a do
  ...
  belongs_to b : B
end

user would get a undefined constant B error.

My guess is that model files are read in alphabetical order and while parsing model A the required model B is not loaded yet.

Workaround: add require "./b" at the top of model A definition file.

Maybe something like this:

column admin : Bool = false

Or should it get it from the database automatically? My thought is that it shouldn't because we're manually defining everything else so it would be weird to have Lucky magically set up a default.

It should probably warn or raise if you forget to the declare the default and there is a default at the database level though.

class TaskForm < Task::BaseForm
  allow title
end

class TaskListForm < TaskList::BaseForm
  allow title
  has_many task_forms : TaskForm
end

# Use in an action
TaskListForm.create(params) do |task_list, form|
  # Handle like a normal form
end

# In a page
text_input @task_list_form.title
@task_list_form.task_forms.each do |task_form|
  text_input task_form.title
end

Some ideas for implementation

I think that a good starting point would be adding many_nested and many_nested? to LuckyRecord::Paramable Basically this would just be an abstract def that would need to then be implemented by LuckyRecord::Params.

Then you'd need to go the lucky repo and implement many_nested in Lucky::Params. For that it should:

• Look for form params in this format: form_name:field_name[position]. Example: tasks:title[0] would be the title for the first task. tasks:body[0] would be the body for the first task. Get it with params.many_nested(:task)
• JSON is much easier. It looks for an array under the given form name. E.g. params.many_nested(:tasks) gets {tasks: [{title: "title", body: "body"}]}
• It treats all JSON fields as string. So: {users: [{age: 30}]} would return [{"age" => "30"}] in Crystal

Prior art

• https://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html
• https://www.pluralsight.com/guides/ruby-on-rails-nested-attributes

Given

class OptionalValuesModel < LuckyRecord::Model
  table optional_values_model do
    column optional_int32 : Int32?
    column optional_int64 : Int64?
    column optional_float64 : Float64?
    column optional_string : String?
    column optional_bool : Bool?
    column optional_time : Time?
    column optional_uuid : UUID?
  end
end
#...
class OptionalValuesModelForm < OptionalValuesModel::BaseForm
  fillable :optional_int32, :optional_int64, :optional_float64, :optional_string, :optional_bool, :optional_time, :optional_uuid 
end

it "coerces blank strings to nil values" do
  form = OptionalValuesModelForm.new(
    {
      "optional_int32" => "",
      "optional_int64" => "",
      "optional_float64" => "",
      "optional_string" => "",
      "optional_bool" => "",
      "optional_time" => "",
      "optional_uuid" => ""
    }
  )
  form.optional_int32.value.should be_nil
  form.optional_int64.value.should be_nil
  form.optional_float64.value.should be_nil
  form.optional_string.value.should be_nil
  form.optional_bool.value.should be_nil
  form.optional_time.value.should be_nil
  form.optional_uuid.value.should be_nil
  form.valid?.should be_true
end

However, none of the type extensions (other than for String, of course) knows how to handle blank strings and will be marked invalid as a result, and the test fails. See https://github.com/snadon/unlucky (specifically files related to a Thing model) which demonstrates this in a less abstract way.

Change the Int32.to_db method to return an int directly and update specs as discussed in #120.

I suggest some validations such as validate_size_of, validate_inclusion_of, and perhaps others, only kick in when field is filled, not when blank.
As of now, for example, if is set validate_size_of name min: 3 I get "Can't be too short" error even when field is blank. It seems to take on role of validate_required. Not required behavior especially for optional fields.

With some schemas there might be join tables that don't need a primary key.

user_roles
  user_id : integer
  role_id : integer

With a setup like this, LuckyRecord currently doesn't like this table. It would be cool to be able to specify a model like this for doing joins through this table when you'd never need to query on this table directly.

I'm having trouble tracking this bug down. Here's a crystal play session you can run, as long as you have a database with two tables users and follows:

users   (id, created_at, updated_at)
follows (id, created_at, updated_at, from_id)

require "avram"

Avram::Repo.configure do |settings|
  settings.url = "postgres://localhost/linkybits_development"
end

class User < Avram::Model
  table users do
  end
end

class Follow < Avram::Model
  table follows do
    belongs_to from : User
  end
end

user = User::BaseQuery.new.first
follows = Follow::BaseQuery.new.preload_from

if !follows.size.zero?
  follows.each do |follows|
    # anything
  end
end

The error is just above # anything:

bind message supplies 2 parameters, but prepared statement "" requires 1

Any idea?

Sometimes people want to write a master and read from one or more replicated database for performance reasons.

Allowing people to override the repos for reads and writes would not be too hard and would make LuckyRecord more suitable for high performance/high availability requirements.

class MasterRepo < LuckyRecord::BaseRepo
end

class ReadRepoOne < LuckyRecord::BaseRepo
end

class ReadRepoTwo < LuckyRecord::BaseRepo
end

abstract class BaseModel
  def repo_for_reads
     # Choose one of the slave repos to read from
     [ReadRepoOne, ReadRepoTwo].sample
  end

  def repo_for_writes
    MasterRepo # Write to master
  end
end

MasterRepo.configure do
  settings.url = # connect to master
end

Especially for the date time helpers in HTML. That way they work out of the box

Currently, when forms are saved methods are called in this order:

• prepare
• validation
• before_save
• before_create / before_update
• triggering method eg. save, create, or update
• after_save
• after_create / after_update

Bold methods are methods that users can hook into.

There are currently no use cases we can think of for before_* callbacks because that method is called after the form is known to be valid.

The thought is, what if we just get rid of them, and add them back if there someone can present a solid use case for them.

Alternatives

• Remove prepare and move the before_* callbacks to they happen before validation
• Remove callbacks and create blocks inside the prepare method to distinguish between create/update steps:

def prepare
  # validations
  on_create do
    # set token
  end
end

• Remove callbacks and have separate prepare_for_create/update methods

If you want to allow a value or nil maybe have a method like is?(something_that_might_be_nil). There may be some other name that is better. Will have to think about it a bit

https://gist.github.com/paulcsmith/2c15093c5bf04fe86b8f3241ab98e490

This needs some more thought before work is started on it

You can drop down to raw crystal-db but it is a bit hairy. It would be nice to do something like this:

luckyframework/lucky_record#56 (comment)

Still need to think it through and get a few more examples to make sure what we come up with is flexible and helpful enough

Hello, i just found this issues on the create form

1. VIRTUAL_FIELDS are not properly passed to the form inside the method.
   They are defined here as arguments:
   https://github.com/luckyframework/lucky_record/blob/6dfa3e2f801bf810cd4998ec36c9a53f51ab30c2/src/lucky_record/needy_initializer_and_save_methods.cr#L66-L79

But they are not assigned to the form.

2. FIELDS constant used on create method is different on BaseForm and ModelForm:
   Model::BaseForm: FIELDS have all fields from model.
   ModelForm < Model::BaseForm.: FIELDS is not defined.

I found this by testing with macros.

3. FIELDS constant is used to generate create methods for the form, so it generates wrong code:
   Model::BaseForm: generate a create method with all fields.
   ModelForm < Model::BaseForm.: generate a create method with none field because its not defined.

My solution

• Assign properly the VIRTUAL_FIELDS to the form.
• Set the FIELDS constant on inherited forms.
• Create a new constant called FILLABLE_FIELDS for generating create methods. Add each field to the constant on the fillable macro.
• Validate NEEDS fields not overlap with FILLABLE_FIELDS.

When attempting to migrate

class CreateListAndListEntry::V20180930082950 < LuckyRecord::Migrator::Migration::V1
  def migrate
    create :list_entries do
      add title : String
      add_belongs_to list : List, on_delete: :do_nothing
      add_belongs_to author : User, on_delete: :cascade
    end
    create :lists do
      add title : String
    end
    alter :users do
      add list_entries : ListEntry, fill_existing_with: :nothing
    end
  end

  def rollback
    drop :list_entries
    drop :lists
    alter :users do
      remove :list_entries
    end
  end
end

I receive the following error message: undefined method 'add_column'

You can find the whole project here https://github.com/dscottboggs/lucky-shopping-list

thanks for the help

Since this name was chosen specifically to highlight the person, might as well add her name and a small quick bio to the README.

https://en.wikipedia.org/wiki/Henriette_Avram

Henriette Davidson Avram (October 7, 1919 – April 22, 2006) was a computer programmer and systems analyst who developed the MARC format (Machine Readable Cataloging), the international data standard for bibliographic and holdings information in libraries.

Inline documentation was purposely left off when things were changing, but now the API is starting to stabilize. Adding documentation to the public facing methods would be fantastic.

If you'd like to help, here is how Crystal docs work: https://crystal-lang.org/docs/conventions/documenting_code.html

Feel free to make small PRs so it's easy to review and merge them in :D

Thanks so much for the help!

Forgive me if I am just not seeing the obvious! I would like to do

PostQuery.new.title("A").or.content("B")

and expecting something like WHERE title = "A" OR content = "B".

Reproduce
Put a nullable has_one field to model
Make it fillable for the form (***_id)
make a visible form with text_input
submit the form with blank text_input
and LuckyForm complains the validity of the field.

I hope it can respect the nullable nature of the model.

This still needs some thought. Please don't work on this yet

It can be confusing when using forms in this scenario

https://gitter.im/luckyframework/Lobby?at=59f61f2d5a1758ed0f645e2f

After further thought, I think it should raise a helpful error at runtime, but ALSO raise a compile time error if you try to pass it params and have no allowed any fields

Also consider user params.nested? instead of params.nested! and only raising when you actually try to save. That way you can instantiate a form without nested params, but when you save it, it'll give you an error if the form name is not there.

I specify the order as follows

query.tx_date.desc_order.id.desc_order

but generated query is

ORDER BY transactions.tx_date, transactions.id DESC 

this query means

ORDER BY transactions.tx_date ASC, transactions.id DESC 

says in this document
https://www.postgresql.org/docs/10/queries-order.html

so I fixed this bug.

We ran into luckyframework/lucky_record#272 (review) and finding the issue was a bit tricky because it said the Time we were passing was "invalid" but it wasn't clear why.

I think we should have a debug_info instance var to LuckyRecord::Field that is an array of strings. This way you could add error messages when parsing that are useful to developers. For example, it would have added: Invalid ("2018-08-08T16:00:00.000Z"): "Time format did not include time zone and no default location provided"

@edwardloveall What do you think? This way when someone p the_field it will show you more helpful information

• How to handle migrations? #137
• Allow specifying separate read/write databases per model
• How to configure multiple database

Ideas for migrations

PrimaryDatabase.configure do |settings|
  settings.url = "some_url"
  settings.migration_folder = "db/migrations/primary" # default is "db/migrations"
end

SpecialDatabase.configure do |settings|
  settings.url = "some_url"
  settings.migration_folder = "db/migrations/special"
end

Then the Migration file can look at the file path to determine which repo it is for.

Configure database

abstract class BaseModel < Avram::Model
  def self.database
     PrimaryDatabase
  end
end

Prior art

https://edgeguides.rubyonrails.org/active_record_multiple_databases.html

Given the example from the guides:

class Tagging < BaseModel
  table :taggings do
    belongs_to tag : Tag
    belongs_to post : Post
  end
end

class Tag < BaseModel
  table :tags do
    column name : String
    has_many taggings : Tagging
    has_many posts : Post, through: :taggings
  end
end

class Post < BaseModel
  table :posts do
    column title : String
    has_many taggings : Tagging
    has_many tags : Tag, through: :taggings
  end
end

it would be great if there was an easy way to assign tags to posts.

For example in ActiveRecord the same example would give you a tag_ids= method on the Post model. When calling post.tag_ids = [1, 3, 5] ActiveRecord will transparently create and / or delete Tagging records to make sure the post is associated with the tags 1, 3 and 5, and only with them. This makes it easy to create HTML forms to edit relationships (either via checkboxes or multiple selects).

I am not 100% sure how this could look in LuckyRecord. Of course it would be super nice to be able to say fillable tag_ids in your form class and have everything else work automatically.

On the other hand: fillable is currently reserved for fields whose values get saved to the actual database table that belongs to the underlying model. So in some ways this would be closer to a virtual field 🤷‍♂️

I just ran into a situation where I needed to do something when I delete a model and realized there aren't callbacks for it. This is because save, create, update exists on the forms which has callbacks, but the models doesn't.

when save empty field it raises error

Should allow save nilable values in database
needed change in src/lucky_record/form.cr:

def set_{{ field[:name] }}_from_param(_value)
  parse_result = {{ field[:type] }}::Lucky.parse(_value)
  if parse_result.is_a? LuckyRecord::Type::SuccessfulCast
    {{ field[:name] }}.value = parse_result.value
  else
    {{ field[:name] }}.add_error "is invalid"
  end
end

to this:

def set_{{ field[:name] }}_from_param(_value)
  parse_result = {{ field[:type] }}::Lucky.parse(_value)
  if parse_result.is_a? LuckyRecord::Type::SuccessfulCast
    {{ field[:name] }}.value = parse_result.value
  else
    {% if field[:nilable] %}
      {{ field[:name] }}.value = nil
    {% else %}
      {{ field[:name] }}.add_error "is invalid"
    {% end %}
  end
end

or cast emty string as nil

Make it so that you can generate an unpersisted model from the form fields

Right now you can clean the database with truncation: luckyframework/lucky_record#137

The problem is it is super slow. Transactions are much faster, so transaction cleaning should be added.

It may be helpful to see how DatabaseCleaner did it: https://github.com/DatabaseCleaner/database_cleaner/blob/master/lib/database_cleaner/active_record/transaction.rb

I have been working on an app with two simple file upload forms. Getting them to work involved a lot of trial and error. One thing I did try, was the following:

class FileForm < LuckyRecord::VirtualForm
  virtual file : Lucky::UploadedFile
  # ...
end

This does, of course, not currently work. But if it did, it would integrate super well with everything else in lucky in a typesafe way. So I started investigating what it would need to make this work.

I think not much is actually needed. But when I started working on a PR, a few questions arose.

First, there is one big caveat: Lucky::UploadedFile is a class from lucky, that lucky_record currently does not depend on. I suspect you would want to keep it that way in order to make lucky_record usable outside of a lucky app.

Two possible workarounds come to mind:

1. Since crystal allows to reopen classes, lucky_record could ship its own (mostly) empty implementation of Lucky::UploadedFile.
2. Introduce a new module similar to LuckyRecord::Paramable that defines the interface for uploaded file objects. This could then be included in lucky into Lucky::UploadedFile.

The second question revolves around LuckyRecord::Paramable. I guess it would need to be extended with four more methods (get_file?, get_file, nested_file?, nested_file). These in turn would then have to be implemented in LuckyRecord::Params. I guess the ? variants could always just return nil, but what would the other two do? Raise an error? If yes, which one?

I would love some feedback.

Related to luckyframework/lucky_record#247 and #52

https://github.com/luckyframework/lucky_record/blob/f3990dd4113234c900c6cc34313283b46ab66190/src/lucky_record/repo.cr#L29-L31

The migration tracking table in LuckyMigrator is just migrations, so I think this is a mistake. It probably was changed on LuckyMigrator but wasn't reflected here.

We support 'Float64', but plain 'Float' should also work.

Probably have a module that shares code between https://gitter.im/luckyframework/Lobby?at=5b3e557f3d8f71623d5f7b74 and a new Float::Lucky class