feat: adiciona suporte para string ISO 8601 em article_publication_date #1057
Conversation
- Permite receber data como string no formato 'YYYY-MM-DD' - Converte automaticamente string para dict com year, month, day - Melhora tratamento de exceção usando XMLWithPreArticlePublicationDateError - Mantém compatibilidade com formato dict existente
There was a problem hiding this comment.
Pull request overview
This PR adds support for ISO 8601 date strings (YYYY-MM-DD format) to the article_publication_date setter in the XMLWithPre class, allowing callers to pass dates as strings in addition to the existing dict format. However, the implementation has several critical issues that need to be addressed before merging.
Key Changes:
- Added string detection and automatic conversion to dict format in
article_publication_datesetter - Modified exception handling from specific to broad catch-all approach
- Changed exception type raised from
ValueErrortoXMLWithPreArticlePublicationDateError
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(value, str): | ||
| parts = value.split("-") | ||
| value = { | ||
| "day": parts[2], | ||
| "month": parts[1], | ||
| "year": parts[0], | ||
| } |
There was a problem hiding this comment.
The string parsing logic is vulnerable to IndexError. If the input string doesn't have exactly 3 parts when split by "-" (e.g., "2024-03" or "2024" or "2024-03-15-extra"), accessing parts[2] will raise an IndexError.
Add validation before accessing the parts:
if isinstance(value, str):
parts = value.split("-")
if len(parts) != 3:
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {value}. Expected ISO 8601 format YYYY-MM-DD"
)
value = {
"day": parts[2],
"month": parts[1],
"year": parts[0],
}| except Exception as e: | ||
| raise XMLWithPreArticlePublicationDateError( |
There was a problem hiding this comment.
Changing the exception type from ValueError to XMLWithPreArticlePublicationDateError breaks the existing tests. Tests at lines 302 and 308 in tests/sps/pid_provider/test_xmlsps.py expect ValueError to be raised for invalid inputs.
Either:
- Keep raising
ValueErrorto maintain backward compatibility, or - Update all affected tests to expect
XMLWithPreArticlePublicationDateError
The PR description claims "✅ Totalmente mantida" (fully maintained) backward compatibility, but this is a breaking change.
| except Exception as e: | ||
| raise XMLWithPreArticlePublicationDateError( | ||
| f"Unable to set article_publication_date with {value}. Date with valid year, month, day is required" | ||
| ) |
There was a problem hiding this comment.
The error message references the potentially modified value variable instead of the original input. If value was converted from a string to a dict, the error message will show the dict representation instead of the original string that was passed in.
Store the original value before modification:
try:
original_value = value
if isinstance(value, str):
parts = value.split("-")
value = {
"day": parts[2],
"month": parts[1],
"year": parts[0],
}
formatted = format_date(**value)
except Exception as e:
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {original_value}. Date with valid year, month, day is required"
)| @article_publication_date.setter | ||
| def article_publication_date(self, value): | ||
| """ | ||
| value : dict (keys: year, month, day) |
There was a problem hiding this comment.
The docstring doesn't document the new string ISO 8601 format support. Update it to reflect the new functionality:
"""
Set article publication date.
Parameters
----------
value : dict or str
If dict: keys are year, month, day
If str: ISO 8601 format (YYYY-MM-DD)
"""| value : dict (keys: year, month, day) | |
| Set article publication date. | |
| Parameters | |
| ---------- | |
| value : dict or str | |
| If dict: keys are year, month, day | |
| If str: ISO 8601 format (YYYY-MM-DD) |
| formatted = format_date(**value) | ||
| except XMLWithPreArticlePublicationDateError: | ||
| raise ValueError( | ||
| except Exception as e: |
There was a problem hiding this comment.
Catching all exceptions with except Exception as e: is too broad and can hide programming errors. The original code specifically caught XMLWithPreArticlePublicationDateError from the format_date function.
The broad exception handler will catch and suppress:
AttributeErrorifvaluedoesn't support expected operationsTypeErrorif incorrect types are passed- Any other unexpected errors
Consider catching only the specific exceptions that format_date and the string parsing can raise:
except (XMLWithPreArticlePublicationDateError, IndexError, ValueError, KeyError) as e:
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {value}. Date with valid year, month, day is required"
)| except Exception as e: | |
| except (XMLWithPreArticlePublicationDateError, IndexError, ValueError, KeyError) as e: |
| if isinstance(value, str): | ||
| parts = value.split("-") | ||
| value = { | ||
| "day": parts[2], | ||
| "month": parts[1], | ||
| "year": parts[0], | ||
| } |
There was a problem hiding this comment.
The new string ISO 8601 format functionality lacks test coverage. Based on the existing test patterns in tests/sps/pid_provider/test_xmlsps.py (lines 290-313), tests should be added for:
- Valid ISO 8601 string (e.g.,
"2024-03-15") - Invalid ISO 8601 formats:
- Missing parts:
"2024-03","2024" - Too many parts:
"2024-03-15-extra" - Invalid separators:
"2024/03/15" - Invalid date values:
"2024-13-01","2024-02-30" - Empty string:
"" - Non-numeric parts:
"abcd-ef-gh"
- Missing parts:
Example test:
def test_article_publication_date_setter_with_iso_string(self):
xml_with_pre = self._get_xml_with_pre("pub", "2023", "1", "9")
xml_with_pre.article_publication_date = "2024-03-15"
self.assertEqual("2024-03-15", xml_with_pre.article_publication_date)| if isinstance(value, str): | ||
| parts = value.split("-") | ||
| value = { | ||
| "day": parts[2], | ||
| "month": parts[1], | ||
| "year": parts[0], | ||
| } |
There was a problem hiding this comment.
The string parsing lacks input validation before splitting and accessing parts. Malicious or malformed inputs could cause unexpected behavior. Consider validating the string format before processing:
if isinstance(value, str):
# Validate basic format before splitting
if not value or not isinstance(value, str):
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {value}. Expected ISO 8601 format YYYY-MM-DD"
)
parts = value.split("-")
if len(parts) != 3:
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {value}. Expected ISO 8601 format YYYY-MM-DD"
)
# Optionally validate that parts are numeric
try:
int(parts[0]) # year
int(parts[1]) # month
int(parts[2]) # day
except ValueError:
raise XMLWithPreArticlePublicationDateError(
f"Unable to set article_publication_date with {value}. Date parts must be numeric"
)
value = {
"day": parts[2],
"month": parts[1],
"year": parts[0],
}Note: format_date will validate the actual date validity, but pre-validation provides clearer error messages.
1 participant
Adiciona suporte para string ISO 8601 em article_publication_date
Descrição
Esta PR adiciona suporte para receber datas no formato string ISO 8601 (
YYYY-MM-DD) no métodoarticle_publication_dateda classeXMLWithPre, mantendo a compatibilidade com o formato dict existente.Motivação
Atualmente, o método
article_publication_dateaceita apenas um dicionário com as chavesyear,montheday. Esta limitação força os consumidores da API a sempre converter strings de data para o formato dict, mesmo quando já possuem datas no formato padrão ISO 8601.Mudanças Implementadas
1. Detecção e conversão automática de strings
YYYY-MM-DDpara o dict esperado internamente2. Melhoria no tratamento de exceções
XMLWithPreArticlePublicationDateErrorde forma consistenteExemplo de Uso
Antes (apenas dict)
Depois (dict ou string)
Testes
Impacto
Checklist
Tipo de mudança