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

Fix length for normalized value #1547

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@tuomoa
Copy link
Contributor

tuomoa commented Oct 17, 2019

Hello @jamesagnew,

There was an issue where a normalized string value couldn't be stored into the database because the normalized string value was too long (over 200 characters). It seems that the normalization which is done with the form Normalizer.Form.NFD may produce longer String than the original one.

The exception was:

Caused by: java.lang.IllegalArgumentException: Value is too long: 201
at ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString.setValueNormalized(ResourceIndexedSearchParamString.java:259) ~[hapi-fhir-jpaserver-model-4.0.3.jar:na]
at ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString.<init>(ResourceIndexedSearchParamString.java:157) ~[hapi-fhir-jpaserver-model-4.0.3.jar:na]

This could be fixed by taking a substring of the normalized value as well before the value is passed on to the ResourceIndexedSearchParamString. I also moved the duplicated methods from different search param extractors to the base class BaseSearchParamExtractor.

Let me know what do you think.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 17, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@12a74e6). Click here to learn what that means.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1547   +/-   ##
=========================================
  Coverage          ?   75.19%           
  Complexity        ?    12679           
=========================================
  Files             ?      923           
  Lines             ?    52006           
  Branches          ?     8707           
=========================================
  Hits              ?    39107           
  Misses            ?     9421           
  Partials          ?     3478
Impacted Files Coverage Δ Complexity Δ
...archparam/extractor/SearchParamExtractorDstu2.java 70.05% <ø> (ø) 70 <0> (?)
...archparam/extractor/SearchParamExtractorDstu3.java 69.89% <ø> (ø) 82 <0> (?)
.../searchparam/extractor/SearchParamExtractorR5.java 42.49% <ø> (ø) 53 <0> (?)
.../searchparam/extractor/SearchParamExtractorR4.java 66.35% <ø> (ø) 86 <0> (?)
...earchparam/extractor/BaseSearchParamExtractor.java 83.33% <80%> (ø) 14 <5> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12a74e6...5c434a3. Read the comment docs.

Copy link
Owner

jamesagnew left a comment

Good call, that didn't occur to me that normalizing could make the string longer.. but that does make sense.

One change requested but overall I think this is a good approach!

@cuhland

This comment has been minimized.

Copy link

cuhland commented Oct 18, 2019

I also stumbled here. Thanks for this PR.
The german character ß (see german sharp s) is in uppercase this: SS

@jamesagnew jamesagnew merged commit 9eddeca into jamesagnew:master Oct 18, 2019
2 of 3 checks passed
2 of 3 checks passed
jamesagnew.hapi-fhir Build #20191018.1 had test failures
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
jamesagnew added a commit that referenced this pull request Oct 18, 2019
@tuomoa tuomoa deleted the tuomoa:fix_normalized_length branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.