Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for nested fields in SPIDERMON_VALIDATION_ERRORS_FIELD #417

Conversation

VMRuiz
Copy link
Collaborator

@VMRuiz VMRuiz commented Aug 31, 2023

Allows to use a nested field as validation field. Nested fields are supported by using . separator:

# settings.py
SPIDERMON_VALIDATION_ERRORS_FIELD = "top_level.second_level._validation"

@VMRuiz VMRuiz requested a review from kmike August 31, 2023 10:40
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 92.59% and project coverage change: +0.09% 🎉

Comparison is base (ea21cee) 79.05% compared to head (4b5a4bf) 79.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
+ Coverage   79.05%   79.14%   +0.09%     
==========================================
  Files          73       74       +1     
  Lines        3151     3175      +24     
  Branches      528      530       +2     
==========================================
+ Hits         2491     2513      +22     
- Misses        590      591       +1     
- Partials       70       71       +1     
Files Changed Coverage Δ
spidermon/contrib/utils/attributes.py 90.47% <90.47%> (ø)
spidermon/contrib/scrapy/pipelines.py 97.80% <100.00%> (+0.07%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -102,7 +102,13 @@ SPIDERMON_VALIDATION_ERRORS_FIELD
Default: ``_validation``

The name of the field added to the item when a validation error happens and
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled.
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled. Nested fields are supported by using `.` separator:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled. Nested fields are supported by using `.` separator:
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled. Nested fields are supported by using ``.`` as a separator:

or

Suggested change
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled. Nested fields are supported by using `.` separator:
`SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS`_ is enabled. Nested fields are supported by using the ``.`` separator:

I wonder if we should acknowledge the lack of support for fields with dots (which I assume are possible), so that users don’t waste time trying escape mechanisms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I thought dots where a good idea because you couldn't define attributes with dots on them. However, it's actually possible to do it with dictionaries {"dotted.key": True}. That means there is a possibility that these new functionality breaks it for such people. For example:

# SPIDERMON_VALIDATION_ERRORS_FIELD = "foo._validation"
a = {"foo._validation" : [] }

Would have worked previously, but now it would raise KeyError because foo is not defined.

Is there a better alternative? Should we allow to escape dots someway? Is this so improbable that is not worth support it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about using something like JMESPath, but while that could work for reading, I am not sure it would for writing (although it might).

Maybe we can go ahead with the dot syntax, mention it in the docs, and if we are worried about breaking code, we could consider using a setting or something to allow disabling nested support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JMESPath is basically dots with nice features, isn't? 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice features including being able to escape dots. But I guess it does break things the same way, only that the solution rather than a setting to disable nesting would be changing the expression to escape dots.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could start with a change that tests fields for dots and logs a warning, and if no one complains about it for some time, then you are validated to introduce this feature. But you might not be willing to wait for a year before adding the feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided there is little to none risk in introducing this as it is.

@VMRuiz VMRuiz merged commit dbd5243 into scrapinghub:master Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants