Feat: IconApi

[tomasciccola]

stacked on top of this PR

closes #333

[gmaclennan]

Looks good to me, let's add some unit tests before merging.

[tomasciccola]

When trying to create tests, I'm getting the following error when creating the instance of DataType:

/home/szgy/data/src/dd/mapeo-core-next/node_modules/better-sqlite3/lib/methods/wrappers.js:5
        return this[cppdb].prepare(sql, this, false);
                           ^
SqliteError: no such table: icon
    at Database.prepare (/home/szgy/data/src/dd/mapeo-core-next/node_modules/better-sqlite3/lib
/methods/wrappers.js:5:21)
    at BetterSQLiteSession.prepareQuery (file:///home/szgy/data/src/dd/mapeo-core-next/node_mod
ules/drizzle-orm/better-sqlite3/index.mjs:17:34)
    at SQLiteSelect.prepare (file:///home/szgy/data/src/dd/mapeo-core-next/node_modules/drizzle
-orm/session-16f863cd.mjs:1112:92)
    at new DataType (file:///home/szgy/data/src/dd/mapeo-core-next/src/datatype/index.js:85:10)
    at file:///home/szgy/data/src/dd/mapeo-core-next/tests/icon-api.js:25:24
    at Test._run (/home/szgy/data/src/dd/mapeo-core-next/node_modules/brittle/index.js:573:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'SQLITE_ERROR'
}

I've done rm -rf drizzle && npm run db:generate:client && npm run db:generate:project and that didn't solve it.
When I regenerate the drizzle stuff it seems the icon table is created.
I'm also using the last version of mapeo-schema and the sqlite-indexer which has icon
@gmaclennan @achou11 any ideas??

[achou11]

@tomasciccola after you create the drizzle instance i.e.

const db = drizzle(sqlite)

you have to run the migration on it to apply the schema like what we do in MapeoProject:

https://github.com/digidem/mapeo-core-next/blob/05b0830d118951fb55ab6b742b242fc0057eecfd/src/mapeo-project.js#L86-L88

[tomasciccola]

@tomasciccola after you create the drizzle instance i.e.

const db = drizzle(sqlite)

you have to run the migration on it to apply the schema like what we do in MapeoProject:

https://github.com/digidem/mapeo-core-next/blob/05b0830d118951fb55ab6b742b242fc0057eecfd/src/mapeo-project.js#L86-L88

Yeah, thanks I was missing that.

Now I'm getting another error when creating the icon. Pasting the extended commit message:

It seems that when calling dataStore.write (wrapped on iconApi.create), the method returns the doc by doing this.getDocById(docId). This seems to return nothing (which is weird, if .write is succesfull, shoudn't .getDocById return something??)

file:///home/szgy/data/src/dd/mapeo-core-next/src/datatype/index.js:140
    if (!result) throw new Error('Not found')
                       ^

Error: Not found
    at DataType.getByDocId (file:///home/szgy/data/src/dd/mapeo-core-next/src/datatype/index.js:140:24)
    at DataType.[kCreateWithDocId] (file:///home/szgy/data/src/dd/mapeo-core-next/src/datatype/index.js:132:17)
    at async IconApi.create (file:///home/szgy/data/src/dd/mapeo-core-next/src/icon-api.js:41:12)
    at async file:///home/szgy/data/src/dd/mapeo-core-next/tests/icon-api.js:36:19
    at async Test._run (/home/szgy/data/src/dd/mapeo-core-next/node_modules/brittle/index.js:573:7)

[achou11]

sounds vaguely familiar. TL;DR: what I was seeing was that i'd be creating a document but it wasn't being indexed because of a bad id I provided, so reading it using the datatype returned nothing.

---

think in my case, i was passing he wrong doc id when calling datatype create, which caused the catch in the index writer here to trigger:

https://github.com/digidem/mapeo-core-next/blob/c9b135d2dafe6d6379aaa5b1305da51e5416197f/src/index-writer/index.js#L62-L68

this causes the batch call here to await okay, which eventually makes the datastore write resolve:

https://github.com/digidem/mapeo-core-next/blob/c9b135d2dafe6d6379aaa5b1305da51e5416197f/src/datastore/index.js#L107

so in the datatype create, the datastore write resolves fine even though the document isn't indexed, which causes the subsequent getByDocId to return nothing

[tomasciccola]

I was forgeting to instance an IndexWriter and doing batch: async(entries) => indexWriter.batch(entries)

[tomasciccola]

Not sure if this implementation makes sense. I'm basically finding the best match by increasing a score on each matching criteria, with the exception of mimeType which is a requirement (so a requested Icon with only matching mimeType will be chosen against an Icon with matching size and pixelDensity
@gmaclennan opinions?

[gmaclennan]

What you say makes sense, the implementation doesn't exactly do that from what I can see:

• If the mimeType does not match, in this implementation you will still get back the first variant (we should probably throw / return not found)
• We probably want the variant that is closest in terms of size and pixel density. With the current implementation anything that has one thing matching (e.g. match score 1) will replace the previous one as the "choice", and not sure that is always the best choice?

This is probably worth unit testing - it's the kind of function that could easily have bugs that are hard to track down in other tests.

[gmaclennan]

@tomasciccola what is needed to fix this and get this merged? Can I help in some way?

[tomasciccola]

Ok. Just to clarify the criteria for this - to see if I'm getting it right:

• Matching mimeType is required, so if asking for something with mimeType: image/png that doesn't have a match, we should throw an Error
• regarding size and pixelDensity, are the two equally important? (in terms of code, do they add the same score when looking for the best match?)
• What happens with a tie? (i.e. we ask for {pixelDensity:1, size:2} and the variants that exist are [{pixelDensity: 1, size:1}, {pixelDensity: 2, size: 2}], which means each variant have one match, resulting en equal score?). We can flip a coin (Math.random() > 0.5) ....

[tomasciccola]

Mh, I realize that flipping a coin shouldn't be a possibility - more so with testing... -. We should probably choose between:

1. Weighting the score of pixelDensity and size differently (would this eliminate ties??)
2. Have a predictable behaviour in case of a tie (choose first? choose last?)

[gmaclennan]

Generally looks good, but the best icon algorythm needs some work. The result depends on variant ordering right now, which is not ideal for testing / getting what we expect. Maybe think through what could be returned and what we actually want.

For the tests, it's just semantics, but your naming and ordering of the arguments for test assertions is the wrong way round - the first argument of t.is() should be the actual data from the test, and the second argument is the expected value (e.g. what you are expecting to get back - not what you get back from the API call). Also best test with buffers, using t.alike(buf, expectedbuf)

[gmaclennan]

What you say makes sense, the implementation doesn't exactly do that from what I can see:

• If the mimeType does not match, in this implementation you will still get back the first variant (we should probably throw / return not found)
• We probably want the variant that is closest in terms of size and pixel density. With the current implementation anything that has one thing matching (e.g. match score 1) will replace the previous one as the "choice", and not sure that is always the best choice?

This is probably worth unit testing - it's the kind of function that could easily have bugs that are hard to track down in other tests.