This project is archived and is in readonly mode.

#1976 ✓stale
pablobm

Scaffold generator not working well with namespaced resources

Reported by pablobm | February 15th, 2009 @ 06:24 PM

Try the following:

./script/generate scaffold admin/something name:string

The generated scaffold does not work out of the box. This happens every time a scaffold is generated within a namespace.

The provided patch solves this problem. It also takes on some refactoring of how the symbols used in the generator templates are generated, using a new Ruby mixin that I have called name_composer.

This name_composer expects to be included in a class with an attribute 'name'. It will then provide methods to generate variants of this name like so:

@@@ Ruby c = Struct.new(:name).new('admin_space/product_line').extend(NameComposer) c.name # => "admin_space/product_line" c.underscored_name # => admin_space/product_line c.plural_base_name # => product_lines c.camelized_name # => AdminSpace::ProductLine



Available modifiers include: base, camelized, underscored, singular and plural.


There is also the question of how models should work. Since ActiveRecord does not automatically pick up a table name admin_space_product_line for model AdminSpace::ProductLine, I have made the generator assume the table name will be product_line.

This could be changed by explicitly naming the table name in the generated model file, but I have left that for discussion for the moment.

Comments and changes to this ticket

  • pablobm

    pablobm February 15th, 2009 @ 06:27 PM

    Oh, blame Lighthouse's not providing a "preview" button. Let me try that piece of code again:

    
    c = Struct.new(:name).new('admin_space/product_line').extend(NameComposer) c.name # => "admin_space/product_line" c.underscored_name # => admin_space/product_line c.plural_base_name # => product_lines c.camelized_name # => AdminSpace::ProductLine
    

    The other two lines affected should be more or less readable anyway...

  • pablobm

    pablobm February 18th, 2009 @ 08:52 PM

    • Title changed from “[patch] Scaffold generator not working well with namespaced resources” to “Scaffold generator not working well with namespaced resources”

    Updated patch file, as overly namespaced resources weren't being generated properly. Includes new test for this case.

  • José Valim

    José Valim March 8th, 2009 @ 09:41 AM

    • Tag changed from generator, master, patch, resource, scaffold to bug, generator, master, patch, resource, scaffold

    Even with Admin::Something instead of admin/something it's not working?

  • Pratik

    Pratik March 9th, 2009 @ 12:57 PM

    • Assigned user set to “Pratik”

    Hey,

    Would it be possible to have the refactoring done in a separate ticket/patch if not too much work ?

    Thanks.

  • pablobm

    pablobm March 15th, 2009 @ 03:27 AM

    @José: It doesn't not work with Admin::Something either.

    @Pratik: that makes sense but... I've been working on that today and have concluded that's a bit difficult to do. Or at least it's difficult that the patches make sense separately.

    All this boils down to the fact that the bits of code that make use of extract_modules() are very confusing. I did both things in parallel because:

    • If I first refactor and then fix the bug, the code after the first patch is still very confusing.

    • On the other hand, if I first fix the bug and then refactor... well, that's the scenario I wanted to avoid by refactoring in the first place.

    Does this sound reasonable?

  • Dinshaw

    Dinshaw March 16th, 2009 @ 08:50 PM

    Sorry if this is a dumb question, but where is the code for the NameCompser?

  • Dinshaw

    Dinshaw March 16th, 2009 @ 09:22 PM

    Ah.. got it. Updated patch doesn't have the name_composer.rb diff info. First one does. If that is how it was meant to be, sorry. I am new to patching and stuff.

  • pablobm

    pablobm March 16th, 2009 @ 11:18 PM

    @Dinshaw: no, it's me the one who did it wrong. Thanks for pointing out :P Shit, I don't know why name_composer is not present in the second patch. I'm going to fix that now.

  • pablobm

    pablobm March 16th, 2009 @ 11:52 PM

    Now, this is the correct patch. Sorry fellas, this was embarrasing...

  • blythe
  • pablobm

    pablobm May 30th, 2009 @ 03:29 AM

    • Tag changed from bug, generator, master, patch, resource, scaffold to 2-3-stable, 2.3.2.1, bug, generator, namespace, patch, resource, scaffold

    Here goes another version of the patch, built upon v2.3.2.1

    This time it's composed of 11 smaller patches, each applying a single (and hopefully fairly simple) change to the code. All tests work after each single patch, and at the end the whole problem is solved.

    The first 9 patches undertake the refactoring mentioned on the original description.

    Patch number 10 adds new tests that show us (lo and behold) that the fix to this bug is halfway there after the initial refactoring.

    Patch number 11 fixes the generation of routes, which is the only item left before solving the issue.

    @Pratik: I hope this is satisfies your request. I didn't create a new ticket because I cannot quite see the refactoring as a ticket (the way I've done it at least), and then the fix as another ticket. Please correct me if wrong.

  • CancelProfileIsBroken

    CancelProfileIsBroken May 30th, 2009 @ 02:18 PM

    +1 - This area of the code is in need of some help, and this set of patches does the trick. I was able to create and use a namespaced scaffold after applying, which was not possible before. The refactoring should also make it possible to tackle some of the other issues in the scaffold generator.

    Patches also apply cleanly on master, and generator appears to work, but generated code isn't working under the current state of actionpack.

  • Pratik

    Pratik May 31st, 2009 @ 09:26 PM

    • Assigned user changed from “Pratik” to “Jeremy Kemper”
  • Michael Koziarski

    Michael Koziarski June 9th, 2009 @ 09:31 AM

    • Milestone changed from 2.x to 2.3.4
  • kmpm

    kmpm July 22nd, 2009 @ 04:22 PM

    Have a look at #1628 which deals with fixtures and namespaces.

  • pablobm

    pablobm July 22nd, 2009 @ 05:37 PM

    I haven't had a look at the patch at #1628 yet, and I'm not sure when I'll be as I just moved house and I still don't have a proper internet connection at my new place.

    However, I shall note that my patches don't generate namespaced models, and therefore don't generate namespaced fixtures either. The way I have chosen to implement it, admin/thing generates a model Thing, as opposed to Admin::Thing.

    This is because AR doesn't seem to automatically recognize admin_things as the appropriate table for an Admin::Thing model, so I decided to go for the simplest working solution. Whether this choice is appropriate or not, I'd love to hear.

  • Kane

    Kane July 22nd, 2009 @ 06:15 PM

    you generate a model without namespace if someone wants to generate one with namespace cause you think this is the simplest solution? LOL

  • pablobm

    pablobm July 23rd, 2009 @ 09:49 AM

    I understand I might not have been entirely clear above, so I'll go into a bit of detail as to what the patches achieve or avoid to do, and why.

    After being patched, upon gently asking the scaffold generator to create "namespace/name", we get:

    • The namespaced controller "Namespace::NamesController" and test file at
    • app/controllers/namespace/names_controller.rb
    • test/functional/namespace/names_controller_test.rb

    • With routing to reach it at /namespace/names

    • Appropriate views at

    • app/views/namespace/names/index.html.erb
    • app/views/namespace/names/show.html.erb
    • app/views/namespace/names/new.html.erb
    • app/views/namespace/names/edit.html.erb

    • A layout at

    • app/views/layouts/namespace/names.html.erb

    • A helper "Namespace::NamesHelper" and a test file at

    • app/helpers/namespace/names_helper.rb
    • test/unit/helpers/namespace/names_helper_test.rb

    • The model "Name" with tests/fixtures/migration:

    • app/models/name.rb
    • test/unit/name_test.rb
    • test/fixtures/names.yml
    • db/migrate/20090723082416_create_names.rb

    And that's it.

    So, from the list above, we see the ONLY thing that is not namespaced is the MODEL. Controller/Views/Helper ARE namespaced, and can be accessed at a namespaced path /namespace/names through a namespaced route.

    Now, why didn't I namespace the model too? Reasons are:

    FIRST, ActiveRecord doesn't play nice with namespaced models. It expects Model "Namespace::Name" to have a table name of "name", instead of "namespace_name".
    The unpatched generator generated the model "Namespace::Name", with migrations/fixtures expecting it to work with a table "namespace_names". This wasn't working at all.

    SECOND, another course of action would have been patching AR too, but I wanted to restrict this to railties for now, before making it more complicated.
    I also could be adding "set_table_name" declarations to any namespaced model, but then that starts to turn ugly.
    So the SIMPLEST course of action was coming up with this solution. After that, feedback/debate here could think otherwise, and the patch could be changed.

    THIRD, personally, this patch scratches my itch. I normally create admin areas under /admin, where I manage models that are used in the public-facing side of the site. For me (and that's only me, I know) the models I work with in the Admin namespace are the same ones used in the rest of the site, and there is not much point in namespacing them at all.

    I hope it is all a little bit more clear now, and less laughable too, for that matter.

  • pablobm

    pablobm July 23rd, 2009 @ 09:49 AM

    Crap, lighthouse messing up with the formatting again. Anyway...

  • Kane

    Kane July 23rd, 2009 @ 04:34 PM

    Okey thanks for the clarification, i often work with namespaces, for models too.
    I would vote for a seperate patch that fixes the ActiveRecord behaviour, so the generators can be patched accordantly.

  • Kane

    Kane July 23rd, 2009 @ 11:09 PM

    the behaviour of 2.3.2 is really interesting..

    ruby script/generate model Site::Article text:text
    ruby script/generate model site/article text:text

    works both which corresponds with the doc
    "Pass the model name, either CamelCased or under_scored"

    but it creates a subfolder and doesnt put the fixtures in it:

    create test/fixtures/site
    create test/fixtures/site_articles.yml

    and to the tablenames..
    its really strange in my opinion

    class CreditCard < ActiveRecord::Base
      class PinNumber < ActiveRecord::Base
      end
    end
    
    assert_equal "credit_card_pin_numbers", CreditCard::PinNumber.table_name
    
    module Billing
      class Account < ActiveRecord::Base
      end
    end
    
    assert_equal 'accounts', Billing::Account.table_name
    

    and it seems this is intended behaviour, i took this from the test cases.
    base_test.rb and modules_test.rb

    can someone explain why the module should not go into the tablename?

  • Michael Koziarski

    Michael Koziarski July 23rd, 2009 @ 11:22 PM

    The module doesn't go in the table name as it's intended 'just' for
    organisational purposes.

    Nesting the class implies ownership, so the table name is prefixed.

    This is intentional.

  • Kane

    Kane July 24th, 2009 @ 05:49 PM

    so the model generator shouldnt build the namespace into the tablename.
    Ive made a patch which adds a test for namespaced model generation and verifies that the migration creats the table correctly.
    I fixed also the mentioned bug with fixture creation.

  • José Valim

    José Valim July 27th, 2009 @ 12:20 PM

    Koz, can I handle this for Rails 3.0?

    1) No namespaces in the table name
    2) Namespace in fixtures directory

    Is it the right way to go?

  • kmpm

    kmpm July 28th, 2009 @ 07:33 AM

    Kane, your fixed generator looks ok but it can't load it's fixtures neither in development or when runing unit tests.

  • Kane

    Kane July 28th, 2009 @ 07:37 PM

    shouldnt be too much work?

    fixtures :all should also load subdirectories

    fixtures :table_name has to find a class with this tablename.
    I assume this is trying to find a class named table_name.singularize. only have to make him to allow namespace before it.

    after this and my patch pablobm can update his patch and the whole story should be history.

    kmpm, i looked at #2965. I will use your tests and look into it.

  • Kane

    Kane July 28th, 2009 @ 07:44 PM

    jose:

    1) is a two line delete.

    2) i assume if namespaces for models are for organisational purpose and are subdirs in /models, in case of the corresponding fixtures it should be the same. it would be inconsistent if a model is in models/namespace/ but the fixture is in fixtures/.

  • José Valim

    José Valim August 10th, 2009 @ 10:22 AM

    • Assigned user changed from “Jeremy Kemper” to “José Valim”
    • Milestone cleared.

    Kane, we need first to solve the fixtures issue before applying the patch. Have you had any success in that front?

  • Kane

    Kane August 13th, 2009 @ 12:29 AM

    i had a look at it. the underlying problem is that the fixture code handles everything through the tablename, and so we have no clue about the namespace cause the only thing that specifies it is the folder in which the fixture is placed.

    another problem is the following scenario if we specify the fixture in the test only with the tablename we can only gamble which fixture file is meant.

    /fixtures/namespace_a/articles.yml

    /fixtures/namespace_b/articles.yml

    class NamespaceA::Article < ActiveRecord::Base
    end
    
    class NamespaceB::Article < ActiveRecord::Base
    end
    
    class WhateverTest < ActiveSupport::TestCase
      fixtures :articles
    end
    

    So we have to rethink this.
    There seems to be no way to avoid generating a set_fixture_class line.

    fixtures :all shouldnt be a problem so it just have to fill both fixture files in the table.

    BUT: what to do if we want to get an article in our tests?
    articles(:first).find has also no chance to detect which model it should use.

  • kmpm

    kmpm August 13th, 2009 @ 05:53 AM

    Kane... In #2965 I also mention the fact that when doing rake db:fixtures:load there isn't a way to do set_fixture_class so I really don't see that as a good workaround.

  • Kane

    Kane August 15th, 2009 @ 08:10 PM

    worked on the patch and it looks good, but:
    where are the tests for rake tasks or how to test db:fixtures:load?

  • José Valim

    José Valim August 15th, 2009 @ 08:53 PM

    The rake tasks usually just invoke a method in a class. The method is properly tested on its framework (for example, fixtures tests on ActiveRecord). And can you please add the patch to another ticket? #2965 already has a patch with tests that you might want to take a look, if it's the same scenario, you could attach it there. :)

  • sebwin

    sebwin September 16th, 2009 @ 01:26 PM

    Well, I really think it would have been nice if, for the time being, pablobm's patch had been incorporated in order to make namespaced scaffolding work as, I would guess, 95% of the users expect it to.

  • José Valim
  • José Valim

    José Valim March 12th, 2010 @ 08:35 PM

    • State changed from “new” to “stale”

    Marking as stale. If this is still a problem or someone is working on it, I will be glad to reopen.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages