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

allow hiding database structure using a column alias #1268

Closed

Conversation

KaulSe
Copy link
Contributor

@KaulSe KaulSe commented Jul 15, 2023

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests and did you add any new tests needed for your feature?
  2. Did you update all templates (if applicable)?
  3. Did you add the relevant documentation (if applicable)?
  4. Did you test locally to make sure your feature works as intended?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

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:

Column::make('User Id', 'data->user->id', 'user_id')
    ->sortable(),

The URL would look like this when sorting for the user id:

?my-table[sorts][user_id]=asc

which is much cleaner than the previous version

?my-table[sorts][data->user->id]=asc

The SQL query will also use the alias;

select "team_integration_feeds"."data"->'user'->>'id' as "user_id" from "team_integration_feeds" order by "user_id" asc limit 10 offset 0

Moreover, this PR should also fix the following error when sorting JSON columns;
15-09-58-15 07 2023

@KaulSe KaulSe changed the title allow hiding database structure using an column alias allow hiding database structure using a column alias Jul 15, 2023
@KaulSe KaulSe marked this pull request as ready for review July 15, 2023 13:36
@lrljoe
Copy link
Collaborator

lrljoe commented Jul 15, 2023

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:
We could easily re-utilise the setSortingPillTitle('CustomTitle') method or the setCustomSlug('CustomSlug') methods and then use the Slug or Custom Title rather than introducing another attribute on the Column

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.

@lrljoe lrljoe changed the base branch from master to develop July 15, 2023 19:57
@KaulSe
Copy link
Contributor Author

KaulSe commented Jul 15, 2023

Ok - will try to do it with setCustomSlug() instead :). will push changes asap

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 15, 2023

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?
Are you MySQL 5.x, MySQL 8.x, Postgres, Mongo, MSSQL for example?

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 15, 2023

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.

@KaulSe
Copy link
Contributor Author

KaulSe commented Jul 15, 2023

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? Are you MySQL 5.x, MySQL 8.x, Postgres, Mongo, MSSQL for example?

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 :).

01-19-13-16 07 2023

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 alias variable, but I removed it from the constructor and use the getAlias(), setAlias() approaches instead.

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.

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 16, 2023

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:

    height_in numeric GENERATED ALWAYS AS (height_cm / 2.54) STORED

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.

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 16, 2023

As an aside, from glancing at your brief example above, it does look like you may be doing some kind of authentication table?
If so - it's worth checking out the Rappasoft LaravelAuthenticationLog package if that's the case, as it provides configurable notifications etc out of the box by monitoring the standard Laravel Events.

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.

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 16, 2023

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!

@lrljoe lrljoe added enhancement New feature or request testing-fix Fix Needs Testing labels Jul 16, 2023
@lrljoe lrljoe self-requested a review July 16, 2023 02:18
@KaulSe
Copy link
Contributor Author

KaulSe commented Jul 16, 2023

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.
As an aside, from glancing at your brief example above, it does look like you may be doing some kind of authentication table?
If so - it's worth checking out the Rappasoft LaravelAuthenticationLog package if that's the case, as it provides configurable notifications etc out of the box by monitoring the standard Laravel Events.

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., user_login, user_register, user_deleted, plugin_installed, etc., ingested through an API endpoint. Each of these events has its own JSON data structure.

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 data column and directly working with that column using PostgreSQL JSON functions.
Maybe one day the time will come and I will refactor all of this, but right now, it isn't worth it :).

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 like your security hat, haha. Databases are not accessible from the outside - and I mostly agree with the security through obscurity point.
Though, I still think a bit differently than the major players in the security industry. I believe that making life more difficult for attackers does improve the security posture, but I bet that saying this will get me in trouble, haha.

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 ?my-table[sorts][data->user->id]=asc does look ugly and is not easy to understand for a user. A clean query string improves that. And obscuring the structure is a small plus point haha.

@lrljoe
Copy link
Collaborator

lrljoe commented Jul 16, 2023

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.

@stale
Copy link

stale bot commented Aug 16, 2023

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.

@stale stale bot added the wontfix This will not be worked on label Aug 16, 2023
@KaulSe
Copy link
Contributor Author

KaulSe commented Aug 16, 2023

Hey / I will look asap :). ATM in Bulgaria

@stale stale bot removed the wontfix This will not be worked on label Aug 16, 2023
@lrljoe
Copy link
Collaborator

lrljoe commented Sep 14, 2023

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

@KaulSe
Copy link
Contributor Author

KaulSe commented Sep 20, 2023

After talking privately with @lrljoe, he gave me a different solution without merging this PR, which would suit my needs.

We can use the setAdditionalSelects in the configure() method with the SQL keyword as to rename a column with an alias.

Example:

in the Model:

    protected $casts = [
        'custom_data' => 'json',
    ];

in configure():

->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):

?newstablePage=1&newstable-sorts[randomdataviews]=asc&newstable-sorts[name]=asc

@KaulSe KaulSe closed this Sep 20, 2023
@lrljoe
Copy link
Collaborator

lrljoe commented Sep 20, 2023

Thanks for confirming, I'll look to add this as an example into the docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing-fix Fix Needs Testing
Projects
Status: Testing/Documentation
Development

Successfully merging this pull request may close these issues.

2 participants