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

As a user, I want the PDS4 properties to be case agnositics #354

Closed
tloubrieu-jpl opened this issue Jun 21, 2023 · 6 comments
Closed

As a user, I want the PDS4 properties to be case agnositics #354

tloubrieu-jpl opened this issue Jun 21, 2023 · 6 comments
Assignees
Labels
B14.0 p.should-have requirement the current issue is a requirement s.high High severity wontfix This will not be worked on

Comments

@tloubrieu-jpl
Copy link
Member

Checked for duplicates

Yes - I've already checked

πŸ§‘β€πŸ”¬ User Persona(s)

API user

πŸ’ͺ Motivation

...so that I can handle the case anyhow and avoid having silent errors when the case is not right.

πŸ“– Additional Details

Actually this is a bug in the current API version where the case is turn to lower case mostly but the q= critiria need to have the exact PDS4 . notation case. I believe it used to work but I don't remember how.

Acceptance Criteria

Given
When I perform
Then I expect

βš™οΈ Engineering Details

No response

@tloubrieu-jpl tloubrieu-jpl added B14.0 needs:triage requirement the current issue is a requirement s.high High severity labels Jun 21, 2023
@jordanpadams jordanpadams added p.should-have and removed s.high High severity labels Jun 26, 2023
@al-niessner
Copy link
Contributor

@tloubrieu-jpl

There is a utility function (model.SearchUtil) that converts . to _ hide the elasticsearch specialness of . -- I do not know if opensearch has the same specialness but given its roots probably. Another utility function does the reverse hiding (mostly) the . and _ translations from the end user.

These functions do not change the case of what is being searched. Part of the reason why is that if we lower cased everything from the user, then the db would have to all be lower case too (opensearch is not case agnostic when it comes to field names). If they were all set to lower case in opensearch, then the user would get them returned as lower case. As I remember that was an undesirable consequence. If you had a table of correct casing for each all lower case field names, it could be undone. However who and how are you maintaining that table? You also have to fix the problem of coexisting field names such as Document and document.

All that said, if it is possible to make opensearch case agnostic to field name case via a global configuration, then that should be done. When searching, case sensitivity of the value can be set but not the field name.

Another option would be to use an edit estimate to get it to select a near field given the edit distance - another way to correct case on the fly -- but it will also change 'cace' to 'case' which may not be desirable or have unwelcome consequences like a successful return when the user was expecting an error.

@tloubrieu-jpl
Copy link
Member Author

Thanks @al-niessner , I believe this worked before. I don't know when and how we lost this feature, maybe when we switched to opensearch... Certainly we did not have to maintain a table to match case sensible properties with lower case properties, and I think I would rather give up that requirement than doing that.

@tloubrieu-jpl
Copy link
Member Author

It might also be that harvest was loading the fields in lower case and stop doing that at some point. Or I just dreamed but the reason I am thinking it used to work is that the notebook I was testing the API with, worked 1 or 2 years ago, then worked again recently except for the case of the properties in the q parameter.

@al-niessner
Copy link
Contributor

@tloubrieu-jpl

Not hallucinating (proof you are not an AI bot). We did make changes for a short time to have the fields be case agnostic but changed back because of the problem of returning wrong case to user in JSON response because all lower case in elasticsearch at the time. I think we did just with the test data at the time. It was short lived. #154 was part of it,

@al-niessner
Copy link
Contributor

@tloubrieu-jpl

May have been changes in how we defined the fields. As text they are case insensitive. Keywords are case sensitive. See:
https://stackoverflow.com/questions/51069213/is-there-a-way-to-make-elasticsearch-case-insensitive-without-altering-the-exist

@tloubrieu-jpl
Copy link
Member Author

tloubrieu-jpl commented Jul 20, 2023

Thanks @al-niessner for pointing that ticket #154 out.

I will close this ticket as wontfix, and open 2 other tickets:

@tloubrieu-jpl tloubrieu-jpl added the wontfix This will not be worked on label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B14.0 p.should-have requirement the current issue is a requirement s.high High severity wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants