EntityFrameworkCore.Jet icon indicating copy to clipboard operation
EntityFrameworkCore.Jet copied to clipboard

We should not support SQL Server specific syntax or features

Open lauxjpn opened this issue 1 year ago • 7 comments

For example, the store type varbinary(max) should not be supported. The Jet equivalent would just be a longbinary.

lauxjpn avatar Mar 07 '24 21:03 lauxjpn

@ChrisJollyAU Please link the PR(s) that fixed this to this issue. Thanks!

lauxjpn avatar Apr 13 '24 11:04 lauxjpn

Didn't need to make any changes as the issue wasn't an issue.

I believe this got open due to a comment I had with my PR for byte array support #228 .

LENB seems to only work properly if column is a longbinary. If has a max length set column type becomes varbinary(5) (if length is 5). In this case the return value is 6. Is this a length+1 or the closest multiple of 2 (because double byte char set as LEN returns 3)

If that was true then changing it all to longbinary would be required but further investigation showed that my original statement was wrong. longbinary and varchar(max) both work fine and LENB has the same behaviour on both

ChrisJollyAU avatar Apr 13 '24 15:04 ChrisJollyAU

I am still finding references for varchar(max) and other (max) suffixes for types in the main project. They are then all getting translated to other store types (without (max)):

  • JetByteArrayMethodTranslator
  • JetByteArrayTypeMapping
  • JetStringMethodTranslator
  • JetTypeMappingSource

Jet does not support the (max) suffix that SQL Server uses, so we should not either, unless there is a very good reason for it. If this is because of the SQL Server tests that were used as the original code base for the test suite, then the tests should be changed to be Jet specific, instead of the the provider to satisfy the SQL Server flavor of the tests.

(There are other SQL Server specific code paths as well. For example, the JetModelExtensions class contains sequence related methods though Jet does not support sequences (AFAIK).)

So this issue should be reopened.

lauxjpn avatar Apr 13 '24 17:04 lauxjpn

Are you talking about in the _storeTypeMappings variable?

That is only used when scaffolding a database (i.e. database first). The model gets created from JetDatabaseModelFactory and the store type is read from that (acquired in AdoxSchema/DaoSchema in GetDataTypeString

This is the value that is later used to match against the _storeTypeMapping. Naturally as the type is coming from the database itself you will never get varbinary(max)

If you want to do anything, just delete the respective lines.

(There are other SQL Server specific code paths as well. For example, the JetModelExtensions class contains sequence related methods though Jet does not support sequences (AFAIK).)

Correct. There are no sequences in Jet. Effectively they are just a no-op. All they do is set or get the annotation, but nowhere actually uses the annotation value

ChrisJollyAU avatar Apr 14 '24 03:04 ChrisJollyAU

Right, those kind of references is what this issue is about. All SQL Server specific syntax references or features that are not part of Jet (meaning not actually implemented or working), should be removed from the codebase.

lauxjpn avatar Apr 14 '24 06:04 lauxjpn

Not really a bug then. More just code that will never be used. Changed tag to code-cleanup

ChrisJollyAU avatar Apr 14 '24 23:04 ChrisJollyAU

Sure, that's a first then, but its a good idea to expand the tag collection.

(It's code that should never be used. Currently, it could be used by any user.)

lauxjpn avatar Apr 15 '24 06:04 lauxjpn