doc_id is preemptively converted to string before being passed to storage class

[cdwilson]

As someone who is brand new to TinyDB, my naive assumption was that when writing to storage, the doc_id would be passed to the storage class keeping the document_id_class type intact, and it would be the responsibility of each storage class to convert it into a type that is compatible for that storage type.

However, I was surprised to find that the Table class preemptively converts the doc_id to a string before passing the table to the storage class for writing:

tinydb/tinydb/table.py

Lines 727 to 736 in 967dbf9

	# Convert the document IDs back to strings.
	# This is required as some storages (most notably the JSON file format)
	# don't support IDs other than strings.
	tables[self.name] = {
	str(doc_id): doc
	for doc_id, doc in table.items()
	}
	# Write the newly updated data back to the storage
	self._storage.write(tables)

I'm curious if there is a reason why this conversion needs to happen in the Table._update_table() method, or whether the conversion could be delegated to the storage class instead?

For example, the YAMLStorage class described in Write a Custom Storage supports writing a YAML document directly to storage with integer document IDs. Making the following change to the Table._update_table() method enables the YAMLStorage storage class to store a YAML document with integer document IDs:

         # Perform the table update operation
         updater(table)
 
-        # Convert the document IDs back to strings.
-        # This is required as some storages (most notably the JSON file format)
-        # don't support IDs other than strings.
-        tables[self.name] = {
-            str(doc_id): doc
-            for doc_id, doc in table.items()
-        }
+        tables[self.name] = table
 
         # Write the newly updated data back to the storage
         self._storage.write(tables)

import yaml
from tinydb import Query, TinyDB
from tinydb.storages import Storage

class YAMLStorage(Storage):
    def __init__(self, filename):
        self.filename = filename

    def read(self):
        with open(self.filename) as handle:
            try:
                data = yaml.safe_load(handle.read())
                return data
            except yaml.YAMLError:
                return None

    def write(self, data):
        with open(self.filename, "w+") as handle:
            yaml.dump(data, handle)

    def close(self):
        pass

db = TinyDB("db.yml", storage=YAMLStorage)
db.insert({"name": "John", "age": 22})

_default:
  1:
    age: 22
    name: John

[MrPigss]

I agree with @cdwilson ,

You can choose the type of the doc_id by setting doc_id_class, but it get converted to string anyway.
Why not let you choose the type of doc_id you actually want to write?
Also even when working with the json format, the moment you choose to use something other than the standard lib json u probably can use other types as key as well.
For example orjson has a nonstringkey option wich allows it to parse things other than string.

Anyhow, this seems more of a job for the storage than for the Table class.

[msiemens]

I'm curious if there is a reason why this conversion needs to happen in the Table._update_table() method, or whether the conversion could be delegated to the storage class instead?

Honestly, the main reason was that when I implemented custom document ID types (Table.document_id_class), I implemeted a necessary type conversion when reading from the storage as some storages (e.g. Python's JSON module) convert dict keys to strings when reading the data. When I got to implement the writing side, it just looked right to me to perform the reverse operation as well – converting document IDs to strings.

A complicating factor regarding this topic is that the interface between TinyDB and storage implementations is under-specified. For a long time the only real contract between TinyDB and storage implementations was that the storage received a dict which may contain any and all kinds of objects. Thus you can store DateTime objects when you're using the MemoryStorage, but you can't do so when using the JSONStorage (at least without using the SerializationMiddleware). This also means that changing how document IDs are represented in the dict that the storage implementation receives may break storages that for some reason depend on the document ID type they receive being a string.

All that being said, a way forward regarding this would be to drop the conversion of document IDs to string when storing data as you proposed and also at least documenting the data that storages may receive from TinyDB better. This still wouldn't solve the issue of different storage implementations supporting different data types. But maybe we could mention this in the user documentation and note that when using non-primitive types (anything not representable in JSON) one may need to use the SerializationMiddleware for some or even most storage implementations.

[msiemens]

Two further notes:

some storages (e.g. Python's JSON module) convert dict keys to strings when reading the data

First, I thought about this and realized that this is just a restriction of using JSON to store data as the JSON specification requires all keys to be strings.

Second, in #460 there is a proposal to get improve the the document ID conversion step when reading data by only applying it when actually returning data but not e.g. when counting items or checking for their presence. This however leads to a question: Do we convert document IDs to strings during writing so reading them is always consistent even for storages that may convert keys to strings like JSON? Or do we always convert document IDs to the document_id_class during reading so TinyDB doesn't break when the JSON storage silently converts keys to strings? Basically, we can optimize for frequent reads or for frequent writes. And I think that in this case, optimizing for frequent reads is the better option as most applications probably will have significantly more reads than writes.

[msiemens]

And for the sake of completeness, dropping all document ID type conversions would not be possible without breaking all storages that for some reason or another convert dict keys during storage. Thus such a step would require a new major version of TinyDB and corresponding changes in all storage implementations that exhibit this behavior.

[cdwilson]

@msiemens thanks for the additional context on this! (sorry for the delayed response, been super busy the last few weeks)

dropping all document ID type conversions would not be possible without breaking all storages that for some reason or another convert dict keys during storage. Thus such a step would require a new major version of TinyDB and corresponding changes in all storage implementations that exhibit this behavior.

Agreed, this occurred to me as well. BTW, this isn't something that I urgently have a need for now, but just wanted to pose the question in case you think it's worth considering for a future version of TinyDB. I was playing around with the YAMLStorage example when I was trying to learn TinyDB, and was curious if anybody else had the need to support storages that have non-string doc_ids.

I'll keep thinking through some of the additional notes you mentioned and reply back once I've had a chance to think through this a bit more.