-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add support for jsonldContext member in subscriptions #1230
feat: add support for jsonldContext member in subscriptions #1230
Conversation
some points are missing in the implementation. for instance, in 5.8.1.4, it is said: (search for |
Test Results 62 files ±0 62 suites ±0 1m 23s ⏱️ +2s Results for commit d928b62. ± Comparison against base commit 2d2e0ff. This pull request removes 190 and adds 38 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
to check if a jsonld context is available or valid do I make an http request to it to make sure it is reachable or do I use it in compaction/expansion to test it... ? |
Doing a compaction or expansion is too expensive. So option 1. The JSON-LD library should already have some functions to do it (this is something it is doing every time we expand / compact). Maybe in https://github.com/filip26/titanium-json-ld/blob/main/src/main/java/com/apicatalog/jsonld/loader/DefaultHttpLoader.java? |
when updating a subscription there are no checks (see #1090 ) should I do a check for the |
Start with the creation only. Will see later. |
4254e8b
to
ba3e0e9
Compare
did you look at the sonar issues? |
subscription-service/src/test/resources/ngsild/subscription_full.json
Outdated
Show resolved
Hide resolved
shared/src/main/kotlin/com/egm/stellio/shared/util/JsonLdUtils.kt
Outdated
Show resolved
Hide resolved
@@ -49,9 +49,11 @@ class NotificationService( | |||
AttributeRepresentation.SIMPLIFIED | |||
else AttributeRepresentation.NORMALIZED | |||
|
|||
val context = it.jsonldContext?.toString()?.let { listOf(it) } ?: it.contexts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- since
context
is a list, call itcontexts
- a bit easier to read:
it.jsonldContext?.let { listOf(it.toString()) } ?: it.contexts
val notificationResult = notificationResults[0] | ||
assertEquals(subscription.id, notificationResult.first.id) | ||
assertEquals(subscription.id, notificationResult.second.subscriptionId) | ||
assertEquals(1, notificationResult.second.data.size) | ||
assertTrue(notificationResult.second.data[0].containsKey(NGSILD_NAME_TERM)) | ||
assertTrue(notificationResult.second.data[0].containsKey(MANAGED_BY_COMPACT_RELATIONSHIP)) | ||
assertEquals(APIC_COMPOUND_CONTEXT, notificationResult.second.data[0][JsonLdUtils.JSONLD_CONTEXT]) | ||
assertTrue(notificationResult.third) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to repeat the assertions of the previous test. here, you want to check what should be different when you have a specific jsonldContext
.
I am also a bit surprised that the entity in the notification has the same keys than the entity in the previous test, whereas they are not compacted with the same contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys are not the same. In the previous entity they are NGSILD_NAME_PROPERTY
and MANAGED_BY_RELATIONSHIP)
. In this entity they are NGSILD_NAME_TERM
and MANAGED_BY_COMPACT_RELATIONSHIP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, my bad
fun checkJsonldContext(context: URI): Either<APIException, Unit> = either { | ||
return try { | ||
context.toString().toUri() | ||
val options = DocumentLoaderOptions() | ||
loader.loadDocument(context, options) | ||
Unit.right() | ||
} catch (e: JsonLdError) { | ||
e.toAPIException(e.cause?.cause?.message).left() | ||
} catch (e: BadRequestDataException) { | ||
e.left() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about:
fun checkJsonldContext(context: URI): Either<APIException, Unit> = either {
return try {
loader.loadDocument(context, DocumentLoaderOptions())
Unit.right()
} catch (e: JsonLdError) {
e.toAPIException(e.cause?.cause?.message).left()
} catch (e: IllegalArgumentException) {
BadRequestDataException(e.cause?.message ?: "Provided context is invalid: $context").left()
}
}
@ranim-n don't forget the last comments in this PR |
have you check getContextsForSubscription() function |
...cription-service/src/main/kotlin/com/egm/stellio/subscription/service/SubscriptionService.kt
Outdated
Show resolved
Hide resolved
LGTM |
fcd1ce8
to
d928b62
Compare
Quality Gate passedIssues Measures |
No description provided.