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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mirage cannot include non-camel cased model attributes when using json-api-serializer #247

Closed
btrude opened this issue Dec 18, 2019 · 19 comments

Comments

@btrude
Copy link

btrude commented Dec 18, 2019

https://github.com/miragejs/miragejs/blob/master/lib/serializers/json-api-serializer.js#L450

Hopefully I'm not misunderstanding something here as I am new to mirage, but the line above explicitly transforms included relationship keys into camel case before checking for their presence on a given model. The attributes on our models are both snake and camel case so this line causes an error when queries attempt to include snake_cased keys. Overriding this function to check for snake and camel cased attributes has resolved our issues for now but perhaps it makes sense to allow for this behavior by default.

@btrude btrude changed the title Mirage cannot include non-camel cased model attributes Mirage cannot include non-camel cased model attributes when using json-api-serializer Dec 18, 2019
@samselikoff
Copy link
Contributor

@btrude Yes – as I wrote in my SO answer, Mirage needs to make assumptions about attribute and relationship name formatting within the confines of Mirage's data layer. The format of your API shouldn't affect how Mirage's data layer works – that's what the Serializer layer is for.

If you're working with Mirage in your tests, for example, you shouldn't do

server.create('user', {
  first_name: 'Barry'
});

just because your API is snake-cased. There are too many formatting decisions around APIs to have them leak into Mirage's model & data layer. For example, APIs sometimes include a root, and sometimes don't. Therefore, Mirage's architecture is designed to keep all of these concerns within the serializer layer, and to keep Mirage's data layer using the camelCase convention everywhere.

So, that line in the code does transform the keys into camelCase, because Mirage is expecting to find them that way.

If your API sends snake-cased attributes for certain models, I would have those models share a customized Serializer that uses the keyFor* hooks to snake-case those attributes. If you need an example of this I am happy to show you! But you definitely shouldn't override _addPrimaryModelToRequestedIncludesGraph as it's not public API and will likely break your app during a future upgrade.

@btrude
Copy link
Author

btrude commented Dec 19, 2019

If your API sends snake-cased attributes for certain models, I would have those models share a customized Serializer that uses the keyFor* hooks to snake-case those attributes. If you need an example of this I am happy to show you! But you definitely shouldn't override _addPrimaryModelToRequestedIncludesGraph as it's not public API and will likely break your app during a future upgrade.

I would definitely appreciate an example because the line I am calling out explicitly overrides any transformations that would have been done in the keyFor* functions making it impossible (as far as my limited understanding tells me) to override this behavior. Our serializer prior to running into this issue already made use of those functions to reconcile the shape of our data with mirage's expectations - it is only when we need to include relationships that are not camel cased on the model that we run into this issue (which admittedly is not often).

@samselikoff
Copy link
Contributor

Sure I can help with an example. Could you post a complete sample JSON response for exactly what you're trying to mock?

@btrude
Copy link
Author

btrude commented Dec 20, 2019

Thank you @samselikoff I really appreciate the help!

In this case we are making a request to:
/api/orders?include=for_reservation,for_reservation.spot,user,order_lines

and expecting to get back something looking like this (attributes removed for readability):

{ 
   "data":[ 
      { 
         "type":"orders",
         "id":"1",
         "attributes":{},
         "relationships":{ 
            "billing_address":{ 
               "data":null
            },
            "broker":{ 
               "data":{ 
                  "type":"users",
                  "id":"12998"
               }
            },
            "cart":{ 
               "data":{ 
                  "type":"carts",
                  "id":"1448700"
               }
            },
            "for_reservation":{ 
               "data":{ 
                  "type":"reservations",
                  "id":"7755412"
               }
            },
            "order_lines":{ 
               "data":[ 
                  { 
                     "type":"order_lines",
                     "id":"1421453"
                  }
               ]
            },
            "originating_partner":{ 
               "data":{ 
                  "type":"partners",
                  "id":"800"
               }
            },
            "user":{ 
               "data":{ 
                  "type":"users",
                  "id":"560972"
               }
            }
         }
      }
   ],
   "included":[ 
      { 
         "type":"spots",
         "id":"26279",
         "attributes":{},
         "relationships":{ 
            "layout":{ 
               "data":{ 
                  "type":"layouts",
                  "id":"3309"
               }
            },
            "classroom":{ 
               "data":{ 
                  "type":"classrooms",
                  "id":"3346"
               }
            },
            "location":{ 
               "data":{ 
                  "type":"locations",
                  "id":"9594"
               }
            },
            "region":{ 
               "data":{ 
                  "type":"regions",
                  "id":"9642"
               }
            },
            "site":{ 
               "data":{ 
                  "type":"sites",
                  "id":"12009"
               }
            },
            "spot_type":{ 
               "data":{ 
                  "type":"spot_types",
                  "id":"3"
               }
            }
         }
      },
      { 
         "type":"reservations",
         "id":"7755412",
         "attributes":{},
         "relationships":{ 
            "booked_on_behalf_of_user":{ 
               "data":{ 
                  "type":"users",
                  "id":"560972"
               }
            },
            "broker":{ 
               "data":{ 
                  "type":"users",
                  "id":"12998"
               }
            },
            "class_session":{ 
               "data":{ 
                  "type":"class_sessions",
                  "id":"389631"
               }
            },
            "credit_transactions":{ 
               "data":[ 
                  { 
                     "type":"credit_transactions",
                     "id":"3195348"
                  }
               ]
            },
            "membership_transactions":{ 
               "data":[]
            },
            "spot":{ 
               "data":{ 
                  "type":"spots",
                  "id":"26279"
               }
            },
            "user":{ 
               "data":{ 
                  "type":"users",
                  "id":"560972"
               }
            }
         }
      },
      { 
         "type":"order_lines",
         "id":"1421453",
         "attributes":{},
         "relationships":{ 
            "order":{ 
               "data":{ 
                  "type":"orders",
                  "id":"1"
               }
            },
            "product":{ 
               "data":{ 
                  "type":"child_products",
                  "id":"7108"
               }
            }
         }
      },
      { 
         "type":"users",
         "id":"560972",
         "attributes":{},
         "relationships":{ 
            "last_region":{ 
               "data":null
            },
            "home_location":{ 
               "data":{ 
                  "type":"locations",
                  "id":"9627"
               }
            }
         }
      }
   ]
}

@samselikoff
Copy link
Contributor

Ok. So you want to camelize both the type name and the relationship names, correct?

(Sorry about the delayed response! Busy holidays.)

@btrude
Copy link
Author

btrude commented Jan 6, 2020

Ok. So you want to camelize both the type name and the relationship names, correct?

(Sorry about the delayed response! Busy holidays.)

@samselikoff No worries, thanks for getting back to me!

The issue is specifically with type names in the array of included relationships but we do also need to camelize type/relationship names across the board.

@samselikoff
Copy link
Contributor

Here's a simplified example:

image

The relevant part is the typeKeyForModel, keyForAttribute and keyForRelationship hooks on the JSONAPISerializer:

application: JSONAPISerializer.extend({
  typeKeyForModel(model) {
    return snakeCase(model.modelName);
  },
  keyForAttribute(key) {
    return snakeCase(key);
  },
  keyForRelationship(key) {
    return snakeCase(key);
  }
})

Does that response look like what you're after?

@btrude
Copy link
Author

btrude commented Jan 6, 2020

Here's a simplified example:

image

The relevant part is the typeKeyForModel, keyForAttribute and keyForRelationship hooks on the JSONAPISerializer:

application: JSONAPISerializer.extend({
  typeKeyForModel(model) {
    return snakeCase(model.modelName);
  },
  keyForAttribute(key) {
    return snakeCase(key);
  },
  keyForRelationship(key) {
    return snakeCase(key);
  }
})

Does that response look like what you're after?

So if I'm understanding correctly we should be setting up our serializer as such and then instantiating factories with camel cased attributes instead of what we were trying to accomplish previously which is instantiating factories with snake case attributes and then using the serializer to camelize them, correct?

@samselikoff
Copy link
Contributor

You got it 👍 Camel case everywhere in Mirage JS code, since it's JS and that's just normal JS convention. Then serializers to turn Mirage's data into the format your app expects based on your production API.

@btrude
Copy link
Author

btrude commented Jan 6, 2020

You got it 👍 Camel case everywhere in Mirage JS code, since it's JS and that's just normal JS convention. Then serializers to turn Mirage's data into the format your app expects based on your production API.

Cool, thanks for clarifying. In trying to implement this in our codebase I'm now running into the issue of our models not having valid associations because we're now passing all of our factory attributes in camelized. Specifically I'm getting the following error:

Mirage: You're trying to create a reservation model and you passed in a model:class-session(95710) under the classSession key, but you haven't defined that key as an association on your model

Is there another serializer hook that I'm missing to handle this case?

@samselikoff
Copy link
Contributor

That error means you’re creating data somewhere with the ORM, and passing in ‘class-session’ as a model name where mirage expects ‘classSession’.

Typically in mirage you create data either with factories in the ‘seeds()‘ method, in tests, or in POST or PUT route handlers. So it could be you or it could be one of Mirage’s shorthands.

Depending on which, you might need to further customize the serializer. But I don’t think you will. It’s probably from a factory somewhere.

@btrude
Copy link
Author

btrude commented Jan 7, 2020

Depending on which, you might need to further customize the serializer. But I don’t think you will. It’s probably from a factory somewhere.

In this case the error is being thrown from within a test like this:

const serverClassSession = server.create('class-session');
server.create('reservation', {
    classSession: serverClassSession,
});

@samselikoff
Copy link
Contributor

Hm ok I don't think dasherize vs. camelCase is the problem here... can you show me your reservation model definition?

@btrude
Copy link
Author

btrude commented Jan 7, 2020

Hm ok I don't think dasherize vs. camelCase is the problem here... can you show me your reservation model definition?

It occurs to me that I should have mentioned at some point that we are using Ember data and that mirage infers all of our models from our ember data models. I would guess that the suggestion here will be to create mirage-specific models that override the snake casing on our ember data models.

@samselikoff
Copy link
Contributor

Hm, yes that might be it. Though I would suggest to refactor your ED models, if it's not too late. Your API format should never leak into your ORM like that, ideally.

@btrude
Copy link
Author

btrude commented Jan 7, 2020

Hm, yes that might be it.

Testing it now and creating a model with camelized attributes for reservation resolves the errors in this case.

Though I would suggest to refactor your ED models, if it's not too late. Your API format should never leak into your ORM like that, ideally.

Completely agree, we're working with older code and an extremely naive implementation of mirage so hopefully we can get to that soon. Thanks again for all your help, I think this should solve everything.

@samselikoff
Copy link
Contributor

Awesome. Good luck + let me know if you need more help!

@saifullah008
Copy link

Capture

i want to remove attributes from response but need data whatever inside it. any idea how to do that?

@cah-brian-gantzler
Copy link
Collaborator

Which attributes do you want to remove? and Im not sure I understand what you mean by need data inside of it. If you need the data, why are you removing them?

The response is crafted by the mirage serializer. For what ever you need most likely you will need to create a serializer and customize that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants