Namespace separator setting #1609

youroff · 2016-03-20T17:46:36Z

Purpose

Introduces a global setting to control namespace separator for generating type attribute when using json-api.
Cool::Author becomes cool--authors with the default value --.
Using custom separator:

ActiveModelSerializers.config.jsonapi_namespace_separator = '/'

This will make for cool/authors which is supported by Ember by default.

Changes

ActiveModelSerializers::Adapter::JsonApi::ResourceIdentifier.type_for

Caveats

As a proposal, we may need to introduce type_transform setting, like key_transform. To provide ways to chose between undersored and dasherized types.

Related GitHub issues

Reimplementation of:
#1216

bf4 · 2016-03-21T02:42:28Z

docs/general/configuration_options.md

+##### jsonapi_namespace_separator
+
+Sets separator string for namespaced models to render `type` attribute. Default value is `--`.
+


An alternative name or perhaps alias would be dashed_namespace_separator (which might also make a nice PR into Rails, to make it easier to configure the replacement string from / to --)

Not sure I understand. It's supposed to be a setting for any custom separator, not only dasherized. Why should it be called dashed_namespace_separator? The point, that I didn't use model_name from rails. Just plain and simple class name processing.

bf4 · 2016-03-21T02:49:33Z

@youroff Awesome, thanks for this!

youroff · 2016-03-21T05:31:46Z

Also, don't you mind if I remove this test in this PR?
https://github.com/rails-api/active_model_serializers/blob/master/test/adapter/json_api/type_test.rb
It seems to be a duplication since ResourceIdentifier has been introduced.

UPD: Disregard this. Probably type_test should be considered as integration.

youroff · 2016-03-23T00:45:47Z

Just ran into some ambiguity in KeyTransform names. Currently it's:

- `:camel` - ExampleKey
- `:camel_lower` - exampleKey
- `:dashed` - example-key
- `:unaltered` - the original, unaltered key
- `nil` - use the adapter default

First off, I suggest to rename :camel_lower into a :snake, I believe it's widely used name for this case.
Second, :unaltered leaves no clues what's it. Probably, would be better to call it :underscore. I made simple method in KeyTransform that makes cross conversion pretty easy, so we could use it with a parameter, everywhere it's needed.

    def transform(string, type)
      string = string.underscore
      case type.to_sym
      when :dashed
        string.dasherize
      when :camel
        string.camelize
      when :snake
        string.camelize(:lower)
      else
        string
      end
    end

What do you think, guys?

bf4 · 2016-03-29T03:43:03Z

lib/active_model_serializers/adapter/json_api/resource_identifier.rb

-            serializer.object.class.model_name.plural
+          type = serializer.object.class.to_s.split('::')
+          type.map! { |t| t.underscore.downcase }
+          type = type.join ActiveModelSerializers.config.jsonapi_namespace_separator


I was thinking this would be a good use-case for gsub, but it looks like that is would be marginally slower and a little harder to read, so just sharing in case it's of interest

Benchmark.realtime { 1000.times { 'AdminUser::Person::THING'.gsub(/[a-zA-Z]+(::|\z)/) do |sub| sub.underscore.downcase.sub('/','--') end } } # => 0.07714433899673168 >> Benchmark.realtime { 1000.times { 'AdminUser::Person::THING'.split('::').map! {|t| t.underscore.downcase }.join('--') } } # => 0.07239663798827678

bf4 · 2016-03-29T03:44:51Z

@youroff I think I'm ok to merge this. Do you feel done with it?

youroff · 2016-03-29T12:06:39Z

There were some not committed additions. Please check last commit in this PR. And probably I need to rebase as there's some conflict already.

bf4 · 2016-04-01T08:31:12Z

@youroff @remear think we can merge this soon?

youroff · 2016-04-03T16:06:52Z

@bf4 Yup, I'll prepare it today

documentation Type transform setting and transformer, name fixed in tests

youroff · 2016-04-04T05:05:03Z

Well, I think this is ready, though CI is failing, not sure why. Any hints?

NullVoxPopuli · 2016-04-04T09:51:50Z

It says spec coverage is not high enough

youroff · 2016-04-04T14:45:40Z

Oh, right, the test for transforms is definitely needed. Will add that soon.

remear · 2016-04-04T14:58:30Z

FYI, I think KeyTransform will soon become https://github.com/remear/active_model_serializers/blob/transforms/lib/active_model_serializers/key_transform.rb via #1645.

With the changes in #1645, I'm having a hard time seeing the reason for the transform method for KeyTransform. If it's something you still want, could you share some more reasoning.

bf4 · 2016-04-04T15:04:47Z

lib/active_model_serializers/adapter/json_api/resource_identifier.rb

-            serializer.object.class.model_name.plural
+          type = serializer.object.class.to_s.split('::')
+          type.map! { |t| KeyTransform.transform(t, ActiveModelSerializers.config.jsonapi_type_transform) }
+          type = type.join ActiveModelSerializers.config.jsonapi_namespace_separator


basically, this method can just piggy back on #1645 right @remear @youroff

that's what i'm thinking

youroff · 2016-04-04T15:19:08Z

I agree, looks good. So should I wait until that PR merged?
I would just rename camel_lower to snake and make sure that it accepts any case. For example:

2.3.0 :001 > "some-key".camelcase
 => "Some-key"

So it may be good idea to underscore string first to make it suitable to cross-conversion in any direction.

youroff · 2016-04-04T15:23:16Z

And I'm not sure what exactly is unaltered. Is there some assumption what is the default format provided by backend? Shouldn't we rely on some particular case per adaptor? Maybe custom lambda in config would do the trick for some weird cases if there are any?

bf4 · 2016-04-04T15:24:28Z

unaltered is the Rails default which is snake_case, but I suppose it's technically whatever as_json or to_json return if someone has customized that

remear · 2016-04-04T15:24:52Z

I'm not sure underscored is an assumption we want to make. Therefore, unaltered is simply the value as it was supplied with no alterations performed.

bf4 · 2016-04-04T15:28:48Z

I'm not sure underscored is an assumption we want to make.

Yeah, I just meant that Rails will default to outputting snake case, which we wouldn't alter

youroff · 2016-04-04T15:30:10Z

Ok, then if we don't have any assumption on what is provided, then we need to underscore everything before conversion, as I mentioned earlier, if provided dasherized string it won't be converted into camel case correctly.

PS: Disregard proposal on snake case, seems like I remember it wrong, wiki says it's underscored.

remear · 2016-04-04T15:35:36Z

Sure, that seems necessary.

For those following along, the problem is:

[1] pry(main)> value = 'some-value'
=> "some-value"
[2] pry(main)> value.underscore
=> "some_value"
[3] pry(main)> value.camelize
=> "Some-value"
[4] pry(main)> value.underscore.camelize
=> "SomeValue"

youroff · 2016-04-04T15:37:37Z

So I remove changes to KeyTransform and wait until that PR is merged to rebase and finish this one?

remear · 2016-04-04T15:48:41Z

I'm adding the underscore changes to KeyTransform now. It would be great if you could look them over in a little bit. We'll then merge in #1645 and this can be rebased to work with those changes.

remear · 2016-04-04T16:14:43Z

@youroff Added -- https://github.com/remear/active_model_serializers/blob/transforms/lib/active_model_serializers/key_transform.rb

youroff · 2016-04-04T16:31:21Z

Hmm.. I think there are couple of potential problems, not severe but they can be easily fixed and code will be shorter. So as far as I see, we don't have configuration validation, so if we call transform through :send it may be insecure. Besides there's a code repetition that could be eliminated if we supplied transformation as a parameter. Something like this:
https://gist.github.com/youroff/f0200c24d8fe168455ee4a779064bce0
What do you think?

remear · 2016-04-04T16:40:35Z

A key (tee hee) reason for the current structure is so users can add custom transforms. Seems like this approach would hinder that ability.

We can continue discussing this using #1645 as a base, which I'll be merging shortly.

bf4 · 2016-04-04T16:40:54Z

so if we call transform through :send it may be insecure

value comes from a config, so it under control, no? there shouldn't be anything going into the send coming from external sources

Unless you mean public_send, which makes no difference since send is a public method

youroff · 2016-04-04T17:00:18Z

Yeah, I meant that user can mess things up himself. That's why I said it wasn't really severe. For custom transforms I would use lambda in config. But I'm ok with either way, just thought it could be nicer.

youroff · 2016-04-04T17:02:18Z

And actually, with proposed implementation it can be extended too, quite easily. We just call super, unless value is String and type is new custom type and provide custom implementation otherwise.

youroff · 2016-04-06T16:26:09Z

There are some breaking changes introduced here:
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/adapter/json_api/resource_identifier.rb#L8
Not sure how to deal with it. I would just remove options from resource identifier, to rely on settings again.
The problem that we can't apply casing transform after separator is applied, cause it can be affected.
Any ideas?

NullVoxPopuli · 2016-05-26T18:49:04Z

@youroff could you rebase? :-)

bf4 · 2016-06-09T08:17:48Z

Merged into master and fixed tests. Submitted merge-result for review before merge into master is pushed #1791

youroff mentioned this pull request Mar 20, 2016

Namespace separator fix for type field in jsonapi adapter #1216

Closed

bf4 reviewed Mar 21, 2016
View reviewed changes

bf4 added C: Feature V: 0.10.x labels Mar 21, 2016

bf4 reviewed Mar 29, 2016
View reviewed changes

bf4 added the C: Namespacing/Versioning label Apr 1, 2016

bf4 assigned remear Apr 1, 2016

youroff force-pushed the namespace_separator_setting branch from 968bbf3 to 9eb2c30 Compare April 4, 2016 04:38

namespace separator setting for json-api and tests

8555c92

documentation Type transform setting and transformer, name fixed in tests

youroff force-pushed the namespace_separator_setting branch from 9eb2c30 to 8555c92 Compare April 4, 2016 04:58

bf4 reviewed Apr 4, 2016
View reviewed changes

bf4 mentioned this pull request Jun 9, 2016

Namespace separator setting for json-api and tests #1791

Closed

bf4 closed this Jun 9, 2016

bf4 reopened this Jun 9, 2016

NullVoxPopuli mentioned this pull request Aug 5, 2016

Namespace separator setting for json-api and tests #1874

Merged

bf4 closed this in #1874 Aug 12, 2016

		##### jsonapi_namespace_separator

		Sets separator string for namespaced models to render `type` attribute. Default value is `--`.

Namespace separator setting #1609

Namespace separator setting #1609

Uh oh!

Conversation

youroff commented Mar 20, 2016

Purpose

Changes

Caveats

Related GitHub issues

Uh oh!

bf4 Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

youroff Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

bf4 commented Mar 21, 2016

Uh oh!

youroff commented Mar 21, 2016

Uh oh!

youroff commented Mar 23, 2016

Uh oh!

bf4 Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

bf4 commented Mar 29, 2016

Uh oh!

youroff commented Mar 29, 2016

Uh oh!

bf4 commented Apr 1, 2016

Uh oh!

youroff commented Apr 3, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

NullVoxPopuli commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

bf4 Apr 4, 2016

Choose a reason for hiding this comment

Uh oh!

remear Apr 4, 2016

Choose a reason for hiding this comment

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

bf4 commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

bf4 commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

remear commented Apr 4, 2016

Uh oh!

bf4 commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

youroff commented Apr 4, 2016

Uh oh!

youroff commented Apr 6, 2016

Uh oh!

NullVoxPopuli commented May 26, 2016