-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
allow hiding database structure using a column alias #1268
allow hiding database structure using a column alias #1268
Conversation
…om/KaulSe/laravel-livewire-tables into allow-hiding-internal-column-names
NOTE: I've rebased this to Develop as that is where we always merge I do really like this idea, but I can see a couple of issues, and potentially a better methodology for doing this. Let me go away and test my theory but I think this can be done using some of the existing methods without needing much work/modification at all. My initial thoughts: If we do end up wanting another attribute for a Query String parameter on a per-Column basis, then we'd want the same approach as everything else with a get method within the Helper trait, and a set method within the Configuration trait, with the variable being attached to the base Column class rather than changing the make() method, as otherwise we'll end up with a Make method a mile long! Take a look at src/Views/Column.php to understand just how many different attributes can be set using the helper, and this, while a great idea, doesn't (to me at least) present anything greater than what's already there that would require changing the make method. The reasoning for the above is purely to ensure consistency and backwards-compatibility, while allowing us to introduce new functionality in a simple way! I really cannot understate enough how awesome it is that you've written the docs and tests to go along with this though, all contribs are really appreciated. |
Ok - will try to do it with setCustomSlug() instead :). will push changes asap |
Thanks. My only other query on it really is - how many people are genuinely pulling back JSON like this, and what are they using for a database backend? Can you let me know what you're using for example? |
You could also use a label here by the way, they're sortable as of the most recent release (about an hour ago), that would also allow you to hide the column(s) from the query string, and you can just use setAdditionalSelects to add the correct column(s) to the query. |
I am storing JSON in a JSONB column in a PostgreSQL database in one of my projects. I don't know how many people use this, though :). For example, given the above JSON payload; Column::make('Username', 'data->data->user_login')
Column::make('User Id', 'data->user->id') My goal is to hide the database structure from the user in the query string and the HTML source... basically limiting the information exposure. Moreover, working with JSON structures lead to ugly query strings, IMO. What do you think about my latest approach? It would still introduce the I was concerned that using the custom slug functionality or the sorting pill title would break existing projects and wouldn't provide this fine-grained control. |
Thanks for the update, I will definitely take a look at it next week. A new attribute, with a set/get sounds like a reasonable approach to me! I know someone else has worked on a PR for multiple columns on the same table using an alias, so I do need to look at what conflicts (if any) may crop up from that! This is definitely not me saying that I won't merge your PR once I've tested it!It looks really good at first glance, and both @rappasoft and I really appreciate the contribution, especially as you've provided tests, docs etc, which is awesome. It may be useful for you (for your project directly), to consider using a generated column in Postgres, I'm not as familiar with them as I am with Virtual Columns in MySQL 8, but I'd assume that they behave in a similar way. For example, you could have a generated column for height in inches that always updates to reflect the correct cm to inch conversion:
MySQL 8.x provides a method for creating a dynamic column from a JSON column, and I'd assume that Postgres has the same. I would always recommend virtual/dynamic columns from JSON columns as appropriate, as it allows your database engine to index the values, and for you to search using the database directly. |
As an aside, from glancing at your brief example above, it does look like you may be doing some kind of authentication table? If not, I would definitely look at what you're storing in your JSON, as really, you should be using some relational models there to cut down on what you're pushing/pulling from your database. Finally, putting my security hat on, security through obscurity is only so valuable (not very!)! If your database is exposed to the internet, fix that first and foremost. Then you can of course consider the Laravel Read DB/Write DB if that's appropriate to reduce the potential for compromise. |
I'm just comparing this and #1190 as they both provide Alias methods. To avoid any possibility of confusion, I spotted that they've used getTableAlias, which is fortunate as it'll prevent me from inevitably getting confused in the future. I can probably get this tested and reviewed sometime next week. I've removed my earlier comment on modifying to setColumnAlias/getColumnAlias, as I think using setAlias/getAlias is probably smoother than setColumnAlias/getColumnAlias, but I'm prepared to be corrected! |
Thank you for the ideas. I have previously used views to "unpack" JSON data in PostgreSQL like this; Schema::createMaterializedView(
'team_urls_technologies_view',
'SELECT jsonb_array_elements_text(technologies) as technology, team_id, asset_id, asset_type, url, id FROM team_urls'
); My table itself contains events, so not only authentication logs, but other kinds of data with a different JSON structure. E.g., I know I could use some other methods, but in my case, I save a lot of engineering time by just putting everything in a
I like your security hat, haha. Databases are not accessible from the outside - and I mostly agree with the security through obscurity point. In that case, I didn't think too much about the security but more about the accessibility for the user and the looks of the application. Having these nested array elements in the URL |
Yep, I'm happy with it being a usability rather than security feature. I'm definitely going to review it later this week and see how it behaves in different demo environments, as it is a great idea. It should in theory make it into the next release. But may end up in the one after. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hey / I will look asap :). ATM in Bulgaria |
Spotted you joined the discord, will pick up with you on there, as it seems to work smoothly in v3 with minimal adjustments at all, so may be a way to get it working in v2 without any headaches |
After talking privately with @lrljoe, he gave me a different solution without merging this PR, which would suit my needs. We can use the Example: in the Model: protected $casts = [
'custom_data' => 'json',
]; in ->setAdditionalSelects(['news.id as id', 'news.custom_data->views as randomDataViews']); In the column configuration: Column::make('randomDataViews')
->label(function ($row) {
return $row->randomDataViews ?? 0;
})
->sortable(function (Builder $query, string $direction) {
return $query->orderBy('custom_data->views', $direction);
}), The query string looks like (lean & clean query string):
|
Thanks for confirming, I'll look to add this as an example into the docs |
All Submissions:
New Feature Submissions:
Changes to Core Features:
JSON fields or relationships lead to unpleasant query strings and expose unnecessary information to a user.
I propose to add an "alias" like the following examples demonstrates:
The URL would look like this when sorting for the user id:
which is much cleaner than the previous version
The SQL query will also use the alias;
Moreover, this PR should also fix the following error when sorting JSON columns;