@Zharktas @pdelboca merged and ready for review again

@pdelboca thanks, how does it look now?

Had a look at this as it directly relates to the work I'm doing on rethinking the search.

This PR looks great but I find the both option and including the validated data in with_custom_schema a bit confusing, and that it complicates the actions code. If the indexing code is the only one that requires both versions of the dict, could we perhaps make two package_show calls in index_update_package(), one with use_default_schema=True and one with use_default_schema=False (both)? Or alternatively validate the dict directly like the old code used to do

For the current version we need to keep things as they are but see #8444 (comment) for some thoughts on data_dict/validated_data_dict going forward

@amercader it's a significant performance hit on datasets with many resources to do all the package_show work twice, that's why I went with slightly uncomfortable "both" option.

@wardi I understand how running package_show twice may affect performance, but if we only focus on validating the data dict, that needs to happen regardless before indexing, right? So if we call package_show once with use_default_schema=True and use_cache=False, and then on index_update_package() we validate it with

 schema = package_plugin.show_package_schema()
 validated_pkg_dict, _errors = lib_plugins.plugin_validate(...)

Wouldn't that take roughly the same time?

My concern other than the added complexity in package_show (mostly, but also in package_create and package_update) is that this is exposed to the public API, so any user can pass use_default_schema=both on a normal package_show call and will get a dict with:

{
    "id": "xxxx",
    "name" "test-dataset",
    # ...
    "validated_data_dict": "<JSON dump of the validated_data_dict>",
    "with_custom_schema": {
       # A copy of the same dict
    }
}

The with_custom_schema is expected, but the validated_data_dict comes from using the cached data_dict, which surprise, surprise also contains a validated_data_dict 😅

I know context vars are frown upon but this seems like a reasonable thing to hide from the public API. And perhaps it would be the perfect time to introduce an explicit for_indexing key as we discussed.

@wardi I understand how running package_show twice may affect performance, but if we only focus on validating the data dict, that needs to happen regardless before indexing, right? So if we call package_show once with use_default_schema=True and use_cache=False, and then on index_update_package() we validate it with

 schema = package_plugin.show_package_schema()
 validated_pkg_dict, _errors = lib_plugins.plugin_validate(...)

Wouldn't that take roughly the same time?

Validating first with the default schema and then with a custom schema isn't the same as validating only with the custom schema, so I couldn't use that approach.

If package_show had a way of returning the raw "dictized" version of the package then we could do something like this.

My concern other than the added complexity in package_show (mostly, but also in package_create and package_update) is that this is exposed to the public API, so any user can pass use_default_schema=both on a normal package_show call and will get a dict with:

{
    "id": "xxxx",
    "name" "test-dataset",
    # ...
    "validated_data_dict": "<JSON dump of the validated_data_dict>",
    "with_custom_schema": {
       # A copy of the same dict
    }
}

The with_custom_schema is expected, but the validated_data_dict comes from using the cached data_dict, which surprise, surprise also contains a validated_data_dict 😅

That's a bug for sure :-)

I know context vars are frown upon but this seems like a reasonable thing to hide from the public API. And perhaps it would be the perfect time to introduce an explicit for_indexing key as we discussed.

If we go with a for_indexing option let's make it a normal parameter. There's nothing that is returned that is internal or tied to access from a plugin so why would we want to prevent someone from e.g. building an external service that indexes datasets.

If we decide to drop the default schema version of data from the search index this all gets much simpler. Do you know if the primary consumer of this data version is the harvest extension or anything else?

@amercader use_default_schema=both has been removed

Thanks @wardi , I think this is a massive improvement and lays the foundation for a newer and better search