camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
42 stars 59 forks source link

Small API modifications to reduce resources consumption #33

Closed patrice-conil closed 2 years ago

patrice-conil commented 2 years ago

Hello everyone, When I implement a facade pattern in front of an API, I expect to be able to do it in a stateless way...just using mapping techniques For our QOD API in front of SCEF this is not possible due to small design decisions.

-SessionInfo => AssessionWithQoSSubscription OK

-NotificationData => QosChanged KO Here the problem comes from the qod parameter that cannot be retrieved from NotificationData. But the requestor must be able to match is initial request with the request id ... so I think we can remove it from QosChanged.

With these small modifications that don't impact functionalities IMHO we can implement stateless service that will be more efficient. What do you think about?

Patrice

eric-murray commented 2 years ago

Hi @patrice-conil

I agree stateless design is an admirable goal. Can I just summarise your proposed changes to ensure I understand them?

You are proposing:

I don't get your point about NotificationData -> QosChanged. A QosChanged schema is defined, but not actually used. Is your proposal to remove this unused schema? I am looking at version 1.2.0 of the API definitions, so it may be that this schema has been used in other versions.

patrice-conil commented 2 years ago

Hi @eric-murray, That's exactly what I'm suggesting. On my side, I have also implemented an event propagation which must map the NotificationData coming from the SCEF into the QosChanged. To be able to forward event in NotificationData to the notificationUri given by caller in the CreateSession, I encode this notificationUri in a path variable of the URI called by the SCEF. This way we wouldn't need to register the notificationUri to call either, because we can retrieve it from the path.

Something like that would do the job


    fun buildNotificationUrl(session: SessionInfo, baseUrl: String): String {
        val url = session.notificationUri?.toURL()
        return if (url == null) "$baseUrl/none" else "$baseUrl/${encodeUri(session.notificationUri)}"
        }
    }

    fun encodeUri(uri: URI): String =
        String(Base64Utils.encodeUrlSafe(uri.toASCIIString().toByteArray(Charset.defaultCharset())))

   @RequestMapping(
        method = [RequestMethod.POST],
        path = ["/qosChanged/{encodedUri}"],
        consumes = ["application/json"]
    )
    fun notificationReceiver(
        @PathVariable("encodedUri") encodedUri: String,
        @RequestBody notificationData: NotificationData
    ): ResponseEntity<Void> {
        val uri = if (encodedUri.equals("none")) encodedUri else String(Base64Utils.decodeUrlSafe(encodedUri.toByteArray(Charset.defaultCharset())))
        val token = uri.substringAfter("authToken=", "none")
        logger.debug("Notification controller called with $uri token=$token: $notificationData")

        notificationService.forwardNotification(URI(uri), notificationData)
        return ResponseEntity.noContent().build()
    }
...

But I think if qos has no interest in this QosChanged the 2 statuses (SESSION_TERMINATION and SUCCESSFUL_RESOURCES_ALLOCATION) will be of great help .

eric-murray commented 2 years ago

Thanks @patrice-conil

So I generally agree with your proposals. But extending notification options beyond the currently specified SESSION_TERMINATED event is a feature extension proposal, and I would suggest to open a new issue to explicitly discuss this.

hdamker commented 2 years ago

@patrice-conil is this issue done with #38 and the PR of v0.8.0?

patrice-conil commented 2 years ago

@hdamker, yes this problem is solved. I missed the fact that we needed a unique ID across multiple platforms.