laravel-trend icon indicating copy to clipboard operation
laravel-trend copied to clipboard

Added driver to support for Sql Server

Open tjodalv opened this issue 1 year ago • 7 comments

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

tjodalv avatar Mar 18 '24 09:03 tjodalv

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.

tjodalv avatar Mar 18 '24 10:03 tjodalv

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.

ryanmortier avatar Mar 19 '24 02:03 ryanmortier

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.

tjodalv avatar Mar 19 '24 11:03 tjodalv

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?

ryanmortier avatar Mar 19 '24 12:03 ryanmortier

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

tjodalv avatar Mar 19 '24 20:03 tjodalv

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 avatar Mar 19 '24 20:03 ryanmortier

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?

ryanmortier avatar Mar 22 '24 21:03 ryanmortier