Feat: mapeoProject.importConfig #405
Conversation
src/mapeo-project.js
Outdated
| } | ||
| const zip = await yauzl.open(configPath) | ||
| // check for already present presets and delete them if exists | ||
| // TODO: do the same for fields and icons? |
There was a problem hiding this comment.
should I also delete every field and icon? or does it makes more sense to get the {iconName, iconId} and {fieldName, fieldId} so that when loading presets we directly reference those?
There was a problem hiding this comment.
For now I implemented the first case (basically delete every preset, field and doc). Matching icons of fields against names doesn't seem to be super reliable...
There was a problem hiding this comment.
Yes I would delete every field. For icons it would be nice to re-use them somehow (e.g. checking hashes) but for now let's just delete them all.
tomasciccola
requested review from
gmaclennan and
achou11
and removed request for
gmaclennan
|
There are some files on the config that I don't know what to do with:
|
There was a problem hiding this comment.
Good work on this. There were a few bugs and I got a bit carried away reviewing and fixing, and ended up with with a bit of a restructure which is here: 17e34e0
I haven't tested this, but hopefully can serve as guidance. It creates a readConfig abstraction, so that we don't end up with a huge method in project.importConfig - we can keep the file parsing separate to actually adding the presets to the database.
There were a few bugs that I addressed:
- (not exactly a bug): the code was reading all icons into memory (because they were all added to the
iconsobject), which would be nice to avoid because this could take a lot of memory. field.keyis not the same as theidthat is referenced inpreset.fields.preset.fieldsrefers toObject.keys(presetsFile.fields).- It's important to validate all the data we import, because it could actually be anything. I've added validation functions, and I avoid importing extra properties that we don't know about (by reading from the schema, rather than just spreading the object from the config file)
I've tried to be really strict about validating everything, but I don't throw on everything - I thought it was better to try to import what we can.
Things that throw
- No
presetsprop - No
fieldsprop - A preset names an icon but it doesn't exist
- A preset names a field but it doesn't exist
- Not being able to read the config file at all
Things that do not throw
- Invalid preset, field or icon (although it may cause one of the errors above if the field or icon is referenced)
I'm not sure what to do with these errors / warnings and they aren't returned to the user right now. I think it's a question for the design team about how to present a warning like "We could import the presets but there were errors". Maybe they will decide to throw on any error.
I'll answer your other questions in a comment with inline replies
Good point, I had forgotten about these. We are not yet using these. These are the sprite sheets that Mapbox uses for rendering icons on the map (the json references the icon location within the sprite sheet icons.png). The goal was to render the map with icons for observations, but we can ignore this data for now until we add that feature (and maybe generate the sprite sheet at runtime if we need it) Yes I think it's ok to ignore for now. A question for the design team if we want this to update the project name and/or description, or any other project settings. Could be both useful and dangerous (e.g. could be confusing to see project settings changed when importing a config file) Hmmm yes I had not thought about this. Can you maybe have a think about how we might modify the schema to include this in the field / preset records we store in the database? I don't think it makes sense to store it as a separate data type? This was used in ID Editor (Mapeo territory view) to style certain features on the map in a particular way (we only used it in Waorani mapping as far as I know). I think it's ok to skip this for now. Yes, I think we should throw an error if either an older or a newer version. |
…:digidem/mapeo-core-next into feat/importConfig
Thanks for this! I'll take a look at it. Yeah, I knew I was doing an inefficient way of reading icons (mostly because I needed to group variants/aggregate them by icon name) but I didn't want to mess with generators; I was basically going with the most straight away implementation for starters... Do you think all the deletion stuff should also live on the |
| // if (!deletedPreset) throw new Error('error deleting preset from db') | ||
| // } | ||
| // } | ||
| // check for already present fields and presets and delete them if exist |
There was a problem hiding this comment.
does it make sense to do this here instead of moving it outside to a function in src/config-imprort.js? I left it here since we are using a couple of methods of the class and it felt weird passing them to the function as params...
There was a problem hiding this comment.
I'm fine with it here. Could also add a deleteAll() method to DataType or we could do a quick helper:
async function deleteAll(dataType) {
const deletions = []
for (const doc of await dataType.getMany()) {
deletions.push(dataType.delete(doc.versionId))
}
return Promise.all(deletions)
}
src/mapeo-project.js
Outdated
| }, | ||
| }) | ||
|
|
||
| await config.close() |
There was a problem hiding this comment.
What if we did this a little earlier in the function so we could close the file sooner (and avoid the case where some operation, like $setProjectSettings, fails and we fail to close the file)?
There was a problem hiding this comment.
I realize an issue with this. I made an optimization to avoid reading every file into memory with the caveat that now the zip is read twice (since when reading the file, a generator is created and the consumed when iterating over entries). Basically calling:
const zip = yauzl.open();
const entries = await zip.readEntries();
for(entry of entries){
...
}One is when calling 'readConfig' and another one when consuming *icons().
One option is to call zip.close() inside readConfig() and the other inside *icons() and avoid returning a close() method from readConfig().
Another would be, keeping the close() method that calls
presetsZip.close(); // handle opened on `readConfig()`
iconsZip.close(); // handle opened on `*icons()`There was a problem hiding this comment.
1. update `@types/yauzl-promise` 2. add 'filename' field to `yaulz.Entry` 3. rename `errors()` to `warnings()`, return empty array instead of null 4. `close()` now closes two handles to the zip file: Since now is open twice and only going through relevant entries, for performance reasons 5. clean conditions for validating presets (using `isRecord`) 6. return `configImport.warnings` from `mapeoProject.importConfig`
await exceptions tests
|
I'll plan to re-review this today or tomorrow.
|
1. remove comment regarding wrong name for `filename` in yauzl since fix was already merged upstream 2. remove unnecesary check of warnings.length 3. remove ts-ignore's and any's for better type safety 4. avoid capturing the extension and allow extension case-insensitiveness 5. remove unnecessary `!isRecord()` check Co-authored-by: Evan Hahn <me@evanhahn.com>
|
before merging, I've created an issue here to see if it makes sense implementing |
This should close #401
importConfig