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

Support for raw payloads #50

Closed
daniel-beard opened this issue Oct 2, 2019 · 18 comments · Fixed by #51
Closed

Support for raw payloads #50

daniel-beard opened this issue Oct 2, 2019 · 18 comments · Fixed by #51

Comments

@daniel-beard
Copy link

I occasionally have a requirement where I have raw string payloads that I'd like to send over APNS. I couldn't see a way to pass a string payload with the current set of classes and protocols. Is this something you'd be willing to support?

E.g. I just want to be able to pass something like the following:

{ "aps": { "alert": "hello world", "badge": 9, "sound": "bingbong.aiff"} ,"g": {"l": "appme:///dispatch/us?utm_medium=notification&utm_source=ec&utm_campaign=20171018"}}
@kylebrowning
Copy link
Member

You should be able to conform to APNSwiftNotification and use that.

struct G {
    let l: String
}
struct AcmeNotification: APNSwiftNotification {
    let acme2: [String]
    let g: G
    let aps: APNSwiftPayload

    init(acme2: [String], aps: APNSwiftPayload, g: G) {
        self.acme2 = acme2
        self.aps = aps
        self.g = g
    }
}

@daniel-beard
Copy link
Author

daniel-beard commented Oct 2, 2019

@kylebrowning the structure varies quite a bit, and the source for this comes from external systems. I would conform to APNSwiftNotification, but I don't know the structure ahead of time.

I'd also like to replace our aging, internal rails based push testing tool, which takes a freeform string input from QA peeps.

@kylebrowning
Copy link
Member

@daniel-beard Let me think about that. We need to find a way to some sort of AnyCodable concept.

This might work but im hesitant to depend on third party dependencies.

@weissi
Copy link
Collaborator

weissi commented Oct 2, 2019

Can't we just do

struct SpecialNotification<ExtraPayload: Codable>: APNSwiftNotification {
    let extra: ExtraPayload
    let aps: APNSwiftPayload
}

?

@daniel-beard
Copy link
Author

@weissi how in that case would I create the aps object from that single string payload?

@kylebrowning
Copy link
Member

kylebrowning commented Oct 2, 2019

What if we did

    public func send(withCustomPayload buffer: ByteBuffer, pushType: APNSwiftConnection.PushType, to deviceToken: String, expiration: Date? = nil, priority: Int? = nil, collapseIdentifier: String? = nil, topic: String? = nil) -> EventLoopFuture<Void> {
            let streamPromise = channel.eventLoop.makePromise(of: Channel.self)
            multiplexer.createStreamChannel(promise: streamPromise) { channel, streamID in
                let handlers: [ChannelHandler] = [
                    HTTP2ToHTTP1ClientCodec(streamID: streamID, httpProtocol: .https),
                    APNSwiftRequestEncoder(deviceToken: deviceToken, configuration: self.configuration, bearerToken: self.bearerTokenFactory?.currentBearerToken, pushType: pushType, expiration: expiration, priority: priority, collapseIdentifier: collapseIdentifier, topic: topic),
                    APNSwiftResponseDecoder(),
                    APNSwiftStreamHandler(),
                ]
                return channel.pipeline.addHandlers(handlers)
            }
            
            let responsePromise = channel.eventLoop.makePromise(of: Void.self)
            let context = APNSwiftRequestContext(
                request: buffer,
                responsePromise: responsePromise
            )
            
            return streamPromise.futureResult.flatMap { stream in
                return stream.writeAndFlush(context)
            }.flatMap {
                responsePromise.futureResult
            }
    }

@kylebrowning
Copy link
Member

Then you just construct the byte buffer yourself, but there's no guarantees the payload is legit safe.

@daniel-beard
Copy link
Author

@kylebrowning I think that will work, you don't need the JSONEncoder param though. I'm cool with the payload being able to be malformed.

@kylebrowning
Copy link
Member

Correct I just did it quick and dirty haha.

@kylebrowning
Copy link
Member

kylebrowning commented Oct 2, 2019

I just tested this, and it seems to work just fine, not sure if @weissi Has any other ideas.

public func send<Notification: APNSwiftNotification>(_ notification: Notification, pushType: APNSwiftConnection.PushType, to deviceToken: String, with encoder: JSONEncoder = JSONEncoder(), expiration: Date? = nil, priority: Int? = nil, collapseIdentifier: String? = nil, topic: String? = nil) -> EventLoopFuture<Void> {
    let data: Data = try! encoder.encode(notification)
    
    var buffer = ByteBufferAllocator().buffer(capacity: data.count)
    buffer.writeBytes(data)
    return send(withByteBuffer: buffer, pushType: pushType, to: deviceToken)
}

public func send(withByteBuffer buffer: ByteBuffer, pushType: APNSwiftConnection.PushType, to deviceToken: String, expiration: Date? = nil, priority: Int? = nil, collapseIdentifier: String? = nil, topic: String? = nil) -> EventLoopFuture<Void> {
        let streamPromise = channel.eventLoop.makePromise(of: Channel.self)
        multiplexer.createStreamChannel(promise: streamPromise) { channel, streamID in
            let handlers: [ChannelHandler] = [
                HTTP2ToHTTP1ClientCodec(streamID: streamID, httpProtocol: .https),
                APNSwiftRequestEncoder(deviceToken: deviceToken, configuration: self.configuration, bearerToken: self.bearerTokenFactory?.currentBearerToken, pushType: pushType, expiration: expiration, priority: priority, collapseIdentifier: collapseIdentifier, topic: topic),
                APNSwiftResponseDecoder(),
                APNSwiftStreamHandler(),
            ]
            return channel.pipeline.addHandlers(handlers)
        }
        
        let responsePromise = channel.eventLoop.makePromise(of: Void.self)
        let context = APNSwiftRequestContext(
            request: buffer,
            responsePromise: responsePromise
        )
        
        return streamPromise.futureResult.flatMap { stream in
            return stream.writeAndFlush(context)
        }.flatMap {
            responsePromise.futureResult
        }
}

@weissi
Copy link
Collaborator

weissi commented Oct 2, 2019

@daniel-beard wait, you want to inject an arbitrary string into the JSON?

@kylebrowning
Copy link
Member

@weissi It appears so. Sounds like they attach arbitrary data on the push notification. So aps still exists, but then they use basically what appears to be [String: AnyObject]

@weissi
Copy link
Collaborator

weissi commented Oct 2, 2019

@kylebrowning hmm, if it were actually structured data, I think what I proposed would’ve worked, no? But I think @daniel-beard wants to have the aps object to be an arbitrary sequence of bytes they get from elsewhere. If that is the case, then Codable won’t support that because it’s no longer typed.

I’m happy adding a function like you propose, I think the parameter name should maybe be rawBytes or so instead of withByteBuffer (the word ‘with’ is usually omitted and ‘ByteBuffer’ is the type name already so doesn’t add much value)

@kylebrowning
Copy link
Member

kylebrowning commented Oct 2, 2019

@weissi Main problem is that APNSwiftPayload is not fully Codable but just Encodable so updating that would be API breaking.

Furthermore I went down the route of testing that and couldn't get it to work with structured data. That being said I tried for like 5 minutes.

Finally I believe @daniel-beard is saying that he won't know the input, so it all has to be arbitrary.

@daniel-beard
Copy link
Author

@kylebrowning yes, that's correct. I don't have structured data, and want to be able to directly load logged payloads from disk and send them.

@kylebrowning
Copy link
Member

@daniel-beard If you don't mind, Im going to let #51 marinate for around 24 hours and then cut a new release of 1.2.1

@daniel-beard
Copy link
Author

Sounds good, thanks for the quick responses and implementation, much appreciated!

@kylebrowning
Copy link
Member

closing in favor of PR #51. Can continue discussion there if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants