Skip to content

Added log for avoid silent incoherence between user input and app behaviour - non-existing fields#189

Closed
Intrinsical-AI wants to merge 3 commits into
dataset-generatorfrom
fix/solr-fieldlist-validation
Closed

Added log for avoid silent incoherence between user input and app behaviour - non-existing fields#189
Intrinsical-AI wants to merge 3 commits into
dataset-generatorfrom
fix/solr-fieldlist-validation

Conversation

@Intrinsical-AI

@Intrinsical-AI Intrinsical-AI commented Jul 28, 2025

Copy link
Copy Markdown
Collaborator

Added:

        # Solr default behavior, passing nonexistent fields results in a silent failure with no logging -- added logging
        self.schema_fields = {field['name'] for field in requests.get(urljoin(self.endpoint.encoded_string(), 'schema/fields')).json()['fields']}
        log.debug(f"Schema fields loaded: {len(self.schema_fields)} fields found.")

This: 1)notice in the logs which fields are present in the index. 2) Subset only the valid ones. Might need aditional logic to handle empty cases. let me know if you agree with the approach.

@Intrinsical-AI

Copy link
Copy Markdown
Collaborator Author

Test are not passing now since we have filtered the fields.. Incoming commit

@Intrinsical-AI

Intrinsical-AI commented Jul 28, 2025

Copy link
Copy Markdown
Collaborator Author

Now test are passing. We stored a set of strings as a class attribute (self.schema_fields, that requests the Solr index (given by the endpoint parameter), the present fields.

Included debug logs following the constructor pattern.

@Intrinsical-AI

Copy link
Copy Markdown
Collaborator Author

@dantuzi Lmk what you think and I'll rebase if needed

# Ask the endpoint the existing fields
schema_resp = requests.get(urljoin(self.endpoint.encoded_string(), 'schema/fields')).json()
# subset only the present fields
self.schema_fields = {field['name'] for field in schema_resp.get('fields', [])}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't see self.schema_fields used somewhere. Am I wrong?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this is needed only in this small piece of code, schema_fields should be a local variable

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it might be useful for checking the payload when building the queries to the engine

# In unit tests the mock response may not contain the expected structure -> store empty set
log.debug(f"Schema fields endpoint did not return expected payload: {exc}; defaulting to empty schema list")
self.schema_fields = set()
log.debug(f"Schema fields loaded: {len(self.schema_fields)} fields found.")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It looks like this entire PR is for writing this string in the logs. Why do you think this is useful?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I as developer / user rather knowing when some fields are present or not, than just executing and expect to behave nice. The problem was the silent omission of fields if not present on the schema

@Intrinsical-AI Intrinsical-AI Jul 31, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Another option could be implementing this directly on the _search method. Maybe makes more sense. But I'd like to know when the searched fields are not present

@dantuzi dantuzi closed this Oct 13, 2025
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.

3 participants