adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.03k stars 189 forks source link

fix: allow camelCase keys in CherryPick during serialization #902

Closed mastermunj closed 1 year ago

mastermunj commented 1 year ago

Proposed changes

This PR handles the scenario mentioned in #896

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

thetutlage commented 1 year ago

I don't think I will be changing the serialisation logic for now. You can use the snake case keys during serialisation.

mastermunj commented 1 year ago

@thetutlage Allow me one more opportunity to elaborate on the use case.

We are working on providing access to our APIs to multiple customers, however, depending on the plan of the customer, the output may contain more information (read properties).

To do this, we are working on a provision to pick the fields and relations on the server side for serialization. We are also implementing type checks to make it easier for devs to decide on the properties to serialize.

Please check the type Serialize and it's usage.

This, I believe, adds to DX.

Arguably we can also do one of the following:

  1. Use snake_case type check for serialization, however, all the model properties are camelCase, and using snake_case for certain cases doesn't seem right for DX.
  2. Convert camelCase to snake_case before calling model.serialize. This, again, adds another layer of processing, which can be avoided.

I believe having a provision to accept camelCase keys for the fields along with snake_case wouldn't harm anyone.

Your thoughts?

mastermunj commented 1 year ago

Hi @thetutlage, please help with your thoughts. I will appreciate it if you can reconsider allowing camelCase as well in the serialize method.

mastermunj commented 1 year ago

Hi @thetutlage, I humbly request you to please consider this PR.

serialize expects relations to be in camelCase but the fields are required to be in snake_case.

I believe allowing camelCase fields will not cause any harm. It will rather allow consistency.

mastermunj commented 1 year ago

Hi @thetutlage, not sure if you're receiving notifications for my comments. I'd appreciate your valuable input.

While working on creating a helper to convert camelCase fields to snake_case fields, I found one more potential challenge with only snake_case approach. Sometime around May 2021, the naming strategy was introduced, in which there was a breaking change that converted addressLine1 to address_line1 instead of address_line_1. This was reported at https://github.com/adonisjs/core/discussions/2571.

As of now, the solution is to provide serializeAs for addressLine1 in the model as address_line_1, which is then handled by the framework. Handling this as an additional layer will become complex and tedious.

Hence, I once again request you to please consider allowing both snake_case and camelCase fields since it does not harm.