Fix/declarative config no exceptions#8079
Fix/declarative config no exceptions#8079Bhagirath00 wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8079 +/- ##
============================================
+ Coverage 90.20% 90.32% +0.12%
- Complexity 7592 7812 +220
============================================
Files 841 843 +2
Lines 22911 23671 +760
Branches 2288 2428 +140
============================================
+ Hits 20666 21380 +714
- Misses 1529 1570 +41
- Partials 716 721 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return carrier.get(key.toUpperCase(Locale.ROOT)); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This could be safer in case of refactoring: return EnvironmentGetter.class.getName()
| @Nullable | ||
| @Override | ||
| public String get(@Nullable Map<String, String> carrier, String key) { | ||
| if (carrier == null || key == null) { |
There was a problem hiding this comment.
Should key be validated against null? It is not annotated with @Nullable, as carrier is.
| * TRACESTATE}, {@code BAGGAGE}). This getter translates keys to uppercase before looking them up in | ||
| * the carrier. | ||
| */ | ||
| public enum EnvironmentGetter implements TextMapGetter<Map<String, String>> { |
There was a problem hiding this comment.
Is this class intentionally in this PR? I don't see it is used anywhere besides the test
| * TRACESTATE}, {@code BAGGAGE}). This setter translates keys to uppercase before inserting them | ||
| * into the carrier. | ||
| */ | ||
| public enum EnvironmentSetter implements TextMapSetter<Map<String, String>> { |
There was a problem hiding this comment.
I have similar comments as for EnvironmentGetter class
| assertThat(otherProps.getDouble("str_key")).isNull(); | ||
| assertThat(otherProps.getBoolean("str_key")).isNull(); | ||
| assertThat(otherProps.getScalarList("str_key", String.class)).isNull(); | ||
| assertThat(otherProps.getScalarList("str_list_key", Long.class)).isNull(); |
There was a problem hiding this comment.
Maybe it would be good to add also a cases for Boolean and Double list element types?
| } | ||
|
|
||
| @Test | ||
| void wrongType() { |
There was a problem hiding this comment.
How about adding test cases with default returned when wrong type is requested?
No need to bloat the PR description with this type of data which the build checks already tell us ;) |
Bhagirath00
force-pushed
the
fix/declarative-config-no-exceptions
branch
from
43cad9b to
6eed41c
Compare
3 participants
issue: #7929
Description:
This PR updates the
DeclarativeConfigPropertiesAPI to returnnullinstead of throwing aDeclarativeConfigExceptionwhen a property exists but has a different data type than requested (e.g., calling getBoolean on a String value).Changes:
In
DeclarativeConfigProperties.java(API): Updated Javadocs for all getter methods to explicitly state that they return null on type mismatch.In
YamlDeclarativeConfigPropertiesTest.java(Tests): Added comprehensive verification tests to ensure thatgetString,getBoolean,getInt,getStructured, andgetStructuredListall return null correctly when a type mismatch occurs.Verification Results:
All tests in the incubator module passed successfully: