2shady4u / godot-sqlite

GDExtension wrapper for SQLite (Godot 4.x+)
MIT License
956 stars 82 forks source link

All paths through open_db get ".db" appended to the end #55

Closed Lucrecious closed 2 years ago

Lucrecious commented 3 years ago

Environment: All.

Issue description: I used open_db on the SQLite object on a file with an extension that is not .db, and instead of opening up the file that I wanted to open it opened <file_name>.db.

Steps to reproduce:

  1. Create SQLite object
  2. Create DB with extension that is not .db
  3. Call open_db on the file from step (2)

Expected: The file path that I put into the function method is opened Actual: The file with a .db appended to the end was opened instead

Minimal reproduction project: I don't think it is necessary.

Additional context I reported this as a bug because as a user, I expect that open_db will simply open the db from the path I give it. I am aware appending .db is "intended" functionality from looking at the code, however, I was very confused as to why I was getting errors for non-existent tables due to this "feature". Its not obvious in the documentation and the function name does not indicate that .db will be added to the end of the input file path. The issue is that it forces me to make a copy of the original database except with the .db extension which kind of hurts the usage value of the database.

2shady4u commented 3 years ago

@Lucrecious I agree.

This extension is an ancient legacy addition to the codebase that has been there since almost the beginning of the project. A better way might be to check if there's an extension in the path and if yes, don't append anything.

What would be the proper way of addressing missing extensions according to you?

I guess from a user-friendliness standpoint, it makes more sense to just return a well-written error instead if there's no extension. From a backward compatibility standpoint it makes more sense to append .db if no extension is detected. (And maybe return a warning that this might be deprecated in the future?)

What is your opinion on this matter?

Lucrecious commented 3 years ago

I'm not sure really. I think there are a few options and I'll tell you which one I prefer but you may have different priorities.

  1. Change the method to not add in the .db if it's missing, and if this breaks backwards compatibility with users of this asset, then you can output an error message that says something like "File not found. Reminder that in godot-sqlite version x.x+ the open_db method does not add .db to paths omitting the extension. Make sure to add .db to your file paths if necessary.". If you want you can even add an optional arguement to open_db that specifies whether or not you want to ensure the .db extension.
  2. To keep backwards compatibility, you can use an optional argument to open_db to specify that you do not want to ensure the .db extension and to keep the file path as-is.
  3. To keep backwards compatibility, you can add a different method, something like open(path: String) that does not alter the path.

My personal preference is for (1) since it's a relatively easy change for users if it breaks their backwards compatibility and it puts your application in the state it should have been in from the beginning (imo).

My second choice would be (3) but the reason I like (1) is because it keeps your API cleaner which I think it more important than making a very, very mild breaking change.

Thanks for the response!

2shady4u commented 3 years ago

@Lucrecious sorry, I didnt have time yet to fix this issue. I've got an additional question related to database files without extensions. In your experience, are they ever used? 🤔

2shady4u commented 3 years ago

@Lucrecious Good news! I've finally found some time to figure out a way to implement this that I'm happy with. The change can be found in https://github.com/2shady4u/godot-sqlite/commit/6a380dce7632c1574d384e79bc7d22b97e3f618e and should be able to alleviate/resolve the cause of your issue.

Currently it works like this:

This change will be part of the next release (v3.1) and thus I'll leave this issue open until then...

Lucrecious commented 3 years ago

Amazing! Sounds great - thanks. Feel free to close this when its implemented.

MathiasBaumgartinger commented 2 years ago

Very nice to see that this is already implemented. My team and i use a Geopackage (.gpkg), which is a SQLite Container, for procedural rendering of landscapes (https://github.com/boku-ilen/landscapelab). Changing the file-extension for the sole purpose of using this plugin seems quite unnecessary.

The only question left: When will this be released approximately? 😊

kb173 commented 2 years ago

Thank you for fixing this and releasing 3.1! We just tested it in our project that @MathiasBaumgartinger mentioned and it's working just as intended, so feel free to close this issue.

2shady4u commented 2 years ago

Currently waiting for the new version to get approved in the Godot Asset Lib before closing this issue. Unfortunately it seems to be taking a while :cold_sweat:

2shady4u commented 2 years ago

This feature has now been added in https://github.com/2shady4u/godot-sqlite/releases/tag/v3.1 and is available in the Godot Asset Lib! As such I'm now closing this issue...