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

Autoconvert relationship name to snakecase #85

Closed
IlCallo opened this issue Nov 20, 2017 · 15 comments
Closed

Autoconvert relationship name to snakecase #85

IlCallo opened this issue Nov 20, 2017 · 15 comments
Labels

Comments

@IlCallo
Copy link
Contributor

IlCallo commented Nov 20, 2017

Hey there, I found myself in this situation in which all data I return have snake case keys, but relations composed by more than 1 words, by Laravel convention, are accessible only via the camel case name.
When #84 will be fixed I'll probably be able to workaround this, but for some reason I feel that automatically translate relations names to snake case could be the right thing, or at least to give the possibility to do so, maybe with an option on the configuration file.

Another option, which can be viable even if I don't like it that much, is to use a decorator to iterate on every key of the response, detect camel case keys and convert them. It seems a bit inefficient to me, but it's a possibility.
The problem in this case is that while in output we'll have snake case, when defining available relationships or autoloaded relationships we'll still be using camel case, which is inconsistent (in my case) with all other data and includes I'll be returning.

Any other ideas on how to tackle this problem?

@flugg
Copy link
Owner

flugg commented Nov 20, 2017

Hey! You could probably use a custom serializer to achieve the snack case fields:

class CustomSerializer extends Flugg\Responder\Serializers\SuccessSerializer
{
    public function mergeIncludes($transformedData, $includedData)
    {
        foreach (array_keys($includedData) as $key) {
            $includedData[snake_case($key)] = $includedData[$key]['data'];
            unset($includedData[$key]);
        }
        return array_merge($transformedData, $includedData);
    }
}

This is not tested but seems like a valid way of tackling it as we're already iterating over the included fields in the default success serializer. What do you think?

Edit: Updated code with fix

@IlCallo
Copy link
Contributor Author

IlCallo commented Nov 21, 2017

This is a valid option indeed, even if it ignores a part of my issue

The problem in this case is that while in output we'll have snake case, when defining available relationships or autoloaded relationships we'll still be using camel case, which is inconsistent (in my case) with all other data and includes I'll be returning.

Some examples:

protected $relations = ['multipleWordRelation'];
protected $load = ['multipleWordRelation' => TransformerClass::class];
return responder()->success($model)->with('multipleWordRelation')->respond();

given that all the returned data would be with keys in snake case, possibly I prefer to use this like

protected $relations = ['multiple_word_relation'];
protected $load = ['multiple_word_relation' => TransformerClass::class];
return responder()->success($model)->with('multiple_word_relation')->respond();

I can try a PR that automatically search snake_case or camerCase relations name based on a config parameter if you like the idea, otherwise I'll just stick to use mixed snake_case and camelCase as I must do now or find another workaround.

@flugg
Copy link
Owner

flugg commented Nov 27, 2017

Sorry for the late reply. Have been infected by the flu, so been a bit down under :p

I agree with you that it's not ideal and a bit inconsistent. I guess a config option is definitely a valid option. Alternatively, a possibility to add mappings to transformers to rename relationships:

$rename = [
    'multipleWordRelation' => 'multiple_word_relation'
];

Just used $rename in lack of better names. But this would technically also allow you to more easily change the name of relations from the point of view of one specific transformer. For instance, if you have a book model, we might not say the book has a user, but instead an author. Of course, you could use author as a relationship directly on the model - but moving the renaming to the transformer allows for more consistency across the board. The downside is that you would have to remap all multi-words relations, that might be a bit cumbersome.

I'll gladly accept a PR for the configuration key though. I'll have some more time for the package closer to Christmas time, just a bit hectic times at work lately :)

@IlCallo
Copy link
Contributor Author

IlCallo commented Nov 27, 2017

Sorry for the late reply. Have been infected by the flu, so been a bit down under :p
Np, get well soon!

The downside is that you would have to remap all multi-words relations, that might be a bit cumbersome.

This seems a little verbose. A mapping could definitely be used to increase flexibility for the use case you mentioned, but I think it's not the right tool to tackle the relationship research problem I highlighted.

The feature I'm thinking could work like this:

  1. read the relation to load
  2. search the relation
  3. if not found, generate camelCase name (we don't actually know or care if the original was already in camelCase)
  4. search the relation with new name
  5. if not found, search attributes with original name
  6. if not found, search attributes with snake case

I think this could be default behaviour (current code will be catched by the first research) avoiding a new config parameter, but if you want it can just be a boolean which blocks or allows the execution of options from 3 to 6.

I'm probably going to need some enhancements before 15 December so I'll start to resolve the issues I created which does not require touching the core of the library.
Can you put some sort of label on the issues you already started to resolve? I'll avoid those ones.
I guess you are working in local environment because I could not find any recent commit even on dev branch.

@flugg
Copy link
Owner

flugg commented Nov 27, 2017

The relationship methods on the model are always camel cased, right? So perhaps this can be solved by skipping point 2 and always convert the relation to camel case if it contains a _ before fetching relationship from the model, and yet keep the snake cased form in the transformers and responses. This way the conversion to snake case would be transparent to the user and you can change it on a per-transformer basis except for globally through the configurations. I also believe this shouldn't be too hard to fix as we only need to update the connection between the transformer and model.

I'll get to labeling the issues soon, thanks! And yeah, currently working locally - I don't mind pushing stuff up on a dev branch though, I'll get around that as well :)

@IlCallo
Copy link
Contributor Author

IlCallo commented Nov 28, 2017

The relationship methods on the model are always camel cased, right?

I'm not really sure about this... I'm searching the docs but I cannot find it anywhere, that's why I proposed those steps. I cannot remember using snake_case for relationships myself.

you can change it on a per-transformer basis except for globally through the configurations

I'm afraid I don't understand what you are referring to here... Like, adding class properties and config options to enable/disable this behaviour?

I'll get to labeling the issues soon, thanks! And yeah, currently working locally - I don't mind pushing stuff up on a dev branch though, I'll get around that as well :)

Ty, I think this could help me or other people trying to add features to the library

@flugg
Copy link
Owner

flugg commented Nov 28, 2017

Basically, what I'm referring to: instead of a 'convert_relations_to_snake_case' option in the configuration to change it globally, you can instead decide whether or not to use snake case or camel case when you apply the relations on each transformer:

protected $relations [
    'group_member'
]

This allows you to have snake cased relations in one transformer and camel cased in another. (Might be useful if you're progressively upgrading your API one endpoint at a time).

I did some quick researching on the snake case relation methods and it does indeed seem like there's no convention set in stone. However, there's no elegant way to check if a relationship actually exists on a model. We could attempt calling the property and catch the exception if it doesn't work, however, I'm not sure that's gonna work with eager loading of nested relations. $model->load('foo.bar.baz') might fail, but we don't necessarily know if it was foo, bar or baz that failed. There is also the security aspect of calling methods on the model, which might exist and have a side-effect, but not be a relationship at all.

Laravel has a method on the router called singularResourceParameters which you can apply to a service provider (most likely RouteServiceProvider) to change the logic of the router:

Router::singularResourceParameters();

Perhaps something similar can be done, seeing underscored relation names on the model seems like an edge case:

Transformer::snakeCasedRelationMethods();

Not a big fan of the name though, just throwing ideas in the air.

@IlCallo
Copy link
Contributor Author

IlCallo commented Nov 28, 2017

The name is actually pretty accurate and I like the idea, now that I understood it.

However, there's no elegant way to check if a relationship actually exists on a model.

I guess that we can take as assumption that if a method with the searched name is present, it must be a relation. If it is not, maybe we can just let the exception generated from using it as a relationship to bubble up, or catch it and rethrow as a custom exception with a more precise message.

@flugg
Copy link
Owner

flugg commented Nov 28, 2017

Yeah indeed, might be a possibility. I've labeled the issues I've already started to work at locally now

@flugg flugg added the feature label Jan 27, 2018
@flugg
Copy link
Owner

flugg commented Jan 28, 2018

Searching some more on the subject I found out relation method must be camel-cased from v5 and beyond. I, therefore, went with the easy solution of checking for the existence of an underscore and converting to camel case both when eager loading in the TransformBuilder and when loading the relationship and accessing the "include", "load" and "filter" methods of the transformer.

Edit: reference

@flugg flugg closed this as completed Jan 28, 2018
@IlCallo
Copy link
Contributor Author

IlCallo commented Jan 28, 2018

Oh, well, one more question is settled then :D

@Hesesses
Copy link

Hesesses commented Feb 3, 2018

I upgraded to 2.0.14 and I have:

class TodoTransformer extends Transformer
{
    protected $relations = ['todoUser'];

When doing the reponser success respond the relation name is todo_user. I'm not sure is this related to this gh issue, but if it is, how do I get this back to todoUser instead of todo_user?

@flugg
Copy link
Owner

flugg commented Feb 3, 2018

Hey! This was added to v3.0.0 so it shouldn’t be related to this issue.

In fact, I can’t recall any place where we convert anything to snake case in the transformers so this sounds very strange. Did this recently happen? What version did you upgrade from?

@Hesesses
Copy link

Hesesses commented Feb 3, 2018

I also upgraded Laravel 5.4 to 5.5...
I'm not sure which version i had and my composer.json shows "flugger/laravel-responder": "^2.0",

this seems to be something else, case closed :)

@flugg
Copy link
Owner

flugg commented Feb 3, 2018

You can update it to ^3.0.0 now that v3 has been released, but make sure to read the changelog to verify if you need to make any changes. Please let me know if the problem still persists

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

No branches or pull requests

3 participants