Slugify and truncate the default collection name#106
Conversation
| if name := crawler.settings.get("INCREMENTAL_CRAWL_COLLECTION_NAME"): | ||
| return name | ||
| name = get_spider_name(crawler).rstrip("_")[:_MAX_LENGTH] + INCREMENTAL_SUFFIX | ||
| return re.sub(r"[^a-zA-Z0-9_]", "_", name) |
There was a problem hiding this comment.
This one may be problematic for another reason - all spiders with unicode names of the same length would silently get the same collection name, and reuse the fingerprint DB unintentionally.
There was a problem hiding this comment.
If we do not care about human readability of the collection names, we could replace characters with their UTF-8 hexadecimal (e.g. å → C3A5) or some other Unicode character ID.
Otherwise, I think we are back to slugify or at least Unidecode, with the caveat that the results may change over time since we have no control over those libraries. We could pin them in zyte-spider-templates-project, but eventually some user may find this problematic.
We could also use the spider numeric ID, since collections are project-specific. We can extract it easily from the middle of the job ID. To keep backward-compatibility with 0.11, we could do this only on spiders with spider names that are invalid collection names.
There was a problem hiding this comment.
I also wonder what are the restrictions on the spider names. The solution could also be to bring the formats closer together (e.g. add more validation for virtual spider names).
Otherwise, I think we are back to slugify or at least Unidecode
Hm, using text-unidecode could be pretty stable. Unlike unidecode or python-slugify, it didn't have a release in many-many years :)
There was a problem hiding this comment.
Oh, nice! And its maintainer feels familiar :)
There was a problem hiding this comment.
What do you think about just disabling the default name creation and require an explicit collection name?
There was a problem hiding this comment.
Good point. I think it is worth considering. While it is a slightly worse initial user experience, it is the most reliable approach, no need for us to worry about any long-term issues with the default name generation.
We could make it a backward-compatible change by making it required only in the UI with some JSON schema customization, and logging a deprecation error (because warnings are not visible in the UI easily) to encourage users to set it on spiders created with 0.11.
There was a problem hiding this comment.
I'm still unsure about requiring a collection name - it's reliable, but it's an additional step. I wonder how often would user face issues in practice with the current approach (unidecode), and if it worths an additional step.
What do you think about logging a warning if the collection name is not set explicitly, and the resulting name is mangled? With a suggestion to set it explicitly?
There was a problem hiding this comment.
What do you think about logging a warning if the collection name is not set explicitly, and the resulting name is mangled? With a suggestion to set it explicitly?
That should help, yes. Though with the current UI, I wonder how many people will notice the warning. On the other hand, I expect much fewer people to actually run into an issue.
3 participants
Fixes #104