Flowframe / laravel-trend

Generate trends for your models. Easily generate charts or reports.
MIT License
720 stars 74 forks source link

Added driver to support for Sql Server #60

Closed tjodalv closed 1 week ago

tjodalv commented 6 months ago

I've created driver for the SQL server and updated readme file to document driver support.

tjodalv commented 6 months ago

I just notice that there is another pull request that adds support for SQL server. Still, I think this one is better because this one is using CONVERT SQLserver function to format date column and the other one is using FORMAT function which is slower and locale aware. Also the other pull request unnecessary refactor code in Trend service IMO.

ryanmortier commented 6 months ago

I don't think this is working?

I tried your fork today and this is the SQL that was generated:

SELECT
  CONVERT(VARCHAR, created_at, 23) AS date,
  count(*) AS aggregate
FROM
  [ crm ].[ account_notes ]
WHERE
  [ created_at ] BETWEEN 2024 -03 -12 01: 59: 02.687
  AND 2024 -03 -19 01: 59: 02.687
GROUP BY
  [ CONVERT(VARCHAR, created_at, 23) ]
ORDER BY
  [ date ] ASC

Query 1 ERROR at Line 1: : Msg: 102, Line 7, State: 1, Level: 15 Incorrect syntax near '01'.

Manually fixed the query to be working:

SELECT
  CONVERT(VARCHAR, [created_at], 23) AS date,
  count(*) AS aggregate
FROM
  [crm].[account_notes]
WHERE
  [created_at] BETWEEN '2024-03-12 01:59:02.687'
  AND '2024-03-19 01:59:02.687'
GROUP BY
  CONVERT(VARCHAR, [created_at], 23)
ORDER BY
  [date] ASC

I removed spaces from inside the brackets, fixed the date strings, and removed the brackets from around the group by clause.

tjodalv commented 6 months ago

I've fixed the problem with groupBy clause when using SQL server driver and tested it against SQL Server 2022. It should work fine now.

ryanmortier commented 6 months ago

Working now, thanks!

SELECT
    CONVERT(VARCHAR, created_at, 23) AS date,
    count(*) AS aggregate
FROM
    [crm].[tickets]
WHERE
    [created_at] BETWEEN '2024-03-12 12:29:28.806'
    AND '2024-03-19 12:29:28.806'
GROUP BY
    CONVERT(VARCHAR, created_at, 23)
ORDER BY
    [date] ASC

Is there a way for you to wrap the column name in brackets in case it's a reserved word?

tjodalv commented 6 months ago

I've push latest commit that encloses date column name in square brackets.

ryanmortier commented 6 months ago

I've push latest commit that encloses date column name in square brackets.

Tested and working great, thanks!

SELECT
    LEFT(CONVERT(VARCHAR, [add_date], 23), 7) AS date,
    sum(scale_quantity) AS aggregate
FROM
    [crm].[tickets]
WHERE
    [commodity_id] = 'corn'
    AND [add_date] BETWEEN '2023-03-19 20:35:42.983'
    AND '2024-03-19 20:35:42.983'
GROUP BY
    LEFT(CONVERT(VARCHAR, [add_date], 23), 7)
ORDER BY
    [date] ASC

@Larsklopstra can we have this reviewed and merged whenever you have a chance?

ryanmortier commented 6 months ago

I got looking at this again today and I started wondering if this could potentially be vulnerable to SQL injection if the developer for some reason accepted input for the ->dateColumn() method?

Larsklopstra commented 1 week ago

Sorry, I won't be support Sql Server OOTB. The new extensible version will allow you to add your own drivers