mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Define Magma::Model.load_attributes #132

Closed alimi closed 4 years ago

alimi commented 4 years ago

This builds on https://github.com/mountetna/magma/pull/123. Magma::Model.load_attributes pulls a model's attributes from the database and defines them on the model using the newly added type column.

With Magma::Model.load_attributes, we'll no longer need to load attribute options when initializing Magma::Attribute objects. This will also give us a little performance boost since we'll get all a model's attributes in one database query. However, we'll no longer be able to merge attributes defined both in Ruby and the database.

This PR also makes more Magma::Attribute options editable, but match is still not supported. It also makes loader and link_model editable which might not be desired. I can remove it--just lemme know 😄.

I'm also not sure where we should call Magma::Model.load_attributes. I could see make adding it explicitly in models like

module Labors
  class Monster < Magma::Model
    parent :labor

    identifier :name, type: String
    # …

    load_attributes
  end
end

so the database attributes take precedence over anything defined in the Ruby files. Alternatively, I could see calling it after the definition to guarantee it's always called.

class Magma
  # …
  class Model
    class << self
      # …
      def load_attributes
        # …
      end

      load_attributes
    end
  end
end

But this would let Ruby attribute definitions override the database definitions. It might not be a big deal since we want to move away from Ruby definitions anyways. I'm curious on what y'all think.

/cc @notmarkmiranda @graft

graft commented 4 years ago

You could have Magma call it in load_models or whatnot, after the models themselves have loaded, this would be in line with #129.

alimi commented 4 years ago

Cool, that makes sense. I added it to the end of Magma::Project#load_project so the db attributes get set after the Ruby attributes. https://github.com/mountetna/magma/pull/132/commits/bc5c12734a573d65a3d95661430b363ee68e1ef3

alimi commented 4 years ago

@notmarkmiranda heads up I added a test here for loading link models from the attributes table.

https://github.com/mountetna/magma/blob/cfc38653df3286c57121460c527f37617c04954d/spec/magma_project_spec.rb#L40-L56

I had this test before https://github.com/mountetna/magma/pull/134, but it was just using link_model instead of link_model_name. It wasn't too hard to resurrect 🧟‍♂️

alimi commented 4 years ago

@graft I have a couple of questions

  1. With this change, if a model has an attribute defined both in Ruby and the attributes table, the Ruby definition will be completely replaced with the db definition. Is this OK? I'm not sure if we'll want to be able to have attributes in both places while we migrate to having everything in the db.
  2. Can you clarify what you mean in this comment: https://github.com/mountetna/magma/pull/132#discussion_r426116616?
graft commented 4 years ago
  1. It seems like this is okay, since we will have the choice of when we replace the Ruby attributes with the db attribute. We might want to write some attribute-auditing code to identify discrepancies before we unhook the models fully.

  2. I don't know what I meant any more. I think I may have been confused about the type being included on updates, but it is only included in the intial insert.