go-fed / apcore

Golang ActivityPub Server Framework
GNU Affero General Public License v3.0
105 stars 10 forks source link

C2S/S2S are not embedded interfaces (iterate on Application & related interfaces) #46

Open cyisfor opened 3 years ago

cyisfor commented 3 years ago

I know golang has a thing where you can go:

type S2SApp interface {
  ...
}
type C2SApp interface {
  ...
}

type Application interface {
  S2SApp
  C2SApp
}

Instead go-fed requires we implement a C2SEnabled/S2SEnabled function. I was thinking of making an AP server that had a GUI interface with no C2S at all, but because Application piles C2S and S2S functions all into one interface, it doesn't matter whether I return false from C2SEnabled. I still have to implement stubs for all the client to server functions, login, oauth, etc, and it still listens to URLs like /login for no reason.

if(C2SEnabled(app)) could always be written more like if(app.(C2SApp) !== nil) unless I seriously misunderstand the Go language, which is entirely possible.

cyisfor commented 3 years ago

okay so maybe I don't understand the language. I still think it doesn't seem right to mix two interfaces together like that when Go works better with their clean separation.

cyisfor commented 3 years ago

right, yeah, so like

type Application interface {
  common-stuff
}
type S2SApp interface {
  Application
  s2s-stuff
}
type C2SApp {
  Application
  c2s-stuff
}
type KitchenSink {
  C2SApp
  S2SApp
}

then I could do like

foo := MyStruct{}
var bar Application = &foo
do-common-stuff;
c2s, ok := bar.(C2SApp)
if(ok) {
  do-c2s-stuff;
}
s2s, ok := bar.(S2SApp)
if(ok) {
  do-s2s-stuff;
}

That'd let us write S2S applications that are not C2S, or C2S applications that aren't S2S though I can't imagine why anyone would want to do that. Test suites?

cjslep commented 3 years ago

(Re-opening the issue)

Thanks for the feedback! Truth be told, I have mainly been refactoring the guts of the application and haven't really focused on iterating on the Application interface itself, so I really appreciate this. I was punting on dealing with this until I (or someone) took #27 (the existing example is currently broken).

The way I envision apcore is to support self-contained ActivityPub applications. That includes use-cases like:

So these questions are more related to "how to pick a strategy for federated software", and less around "how to authorize users".

The latter is more geared towards those authorization flows you mention (/login or OAuth2), which is intended to authorize a particular user for the application; whether that user is using a C2S client or a web client does not matter: /login or OAuth2 can be supported by custom clients / REST clients or, because it is not specified in C2S, used by C2S clients as well. I hope this makes sense, because I don't quite understand your use-case for neither having a /login nor a OAuth2 server: what sort of ActivityPub software would you want to write with apcore that does not involve having users authorize with these sorts of methods? Depending on that answer, it may be a bit out of scope of what I'm attempting to accomplish near term with apcore.

cjslep commented 3 years ago

To elaborate on the history why they are not composable interfaces currently (like, say, how go-fed/activity/pub has composable interfaces):

I began apcore before go-fed/activity hit v1, which included an overhaul of the go-fed/activity/pub interfaces. Because when composing interfaces, Go does not support colliding function signatures, and v0 definitely had that as a problem. Oops! And I had not noticed it because I had not written an app attempting to support C2S and S2S before. An embarassing oversight, but if there was a time to make it, v0 was the perfect time. Only during the development of v1 did I recognize the problem and rectify it.

I just haven't had the time/chance to re-examine the Application interface since.

cjslep commented 3 years ago

On a technical note, from what I learned from go-fed/activity/pub this is a source of headache:

type App interface {}

type C2S interface {
  App
}

type S2S interface {
  App
}

Because an Application will not be able to support both C2S and S2S because their embedded Apps will collide, and the Go compiler will complain.

Instead it will need to be:

type App interface {}

type C2S interface {}

type S2S interface {}

Which permits this composition feature. However, it is slightly annoying for the framework because if a function relies on methods from both App and S2S, it becomes an awkward game of:

func foo(a *App) {
  s, ok := a.(*S2S)
  if !ok { ... }
  a.doSomethingCommon()
  s.doSomethingS2S()
}

or

func foo(a *App, s *S2S) { // `a` & `s` point to the same reference
  a.doSomethingCommon()
  s.doSomethingS2S()
}

or something else.

I'm OK with the framework shouldering the burden of this awkwardness in the Go language, if it means people using apcore can write cleaner code.

aschrijver commented 3 years ago

I am insufficiently skilled in Go to gauge the deeper implications, but here's my 2 cts..

I'm OK with the framework shouldering the burden of this awkwardness in the Go language

Is it really that awkward? You say "a & s point to the same reference" but I assume I only see func's of the specific interface (i.e. not s.doSomethingCommon()). Then its a separation of concerns, and in DDD terms C2S, S2S and App represent different (sub)domains.

Some questions:

What would "shouldering the burden in the framework" entail? Hiding the inconvenience from implementers? You've marked as 'feature'.. does it involve a lot of work at the go-fed end, and would the impact of changes on existing implementers be high?

That includes use-cases like: [..] Someone that wants both.

This is where my use case, Groundwork project, is focused on. It would be positioned as "Built on top of the solid Go-Fed foundation, with [full pub/apcore feature list] and adding 'Dynamic service modules' and 'Service management'..".

cjslep commented 3 years ago

Is it really that awkward?

When Go interfaces are uncomposed from one another, then casting from one interface to another is exactly that: a casting statement. Breaking apart a monolithic interface is going to introduce those statements somewhere, which can indeed be done poorly/awkwardly. I hope to avoid that. :)

Note: in the "composition vs inheritance" debate, Go chose "composition" (there is no inheritance, besides the implicit duck-typing of structs to fulfill interfaces).

You've marked as 'feature'.. does it involve a lot of work at the go-fed end, [...]

It is some work, I wouldn't say a lot, but the way it could be expressed in the codebase is potential for unwieldiness. It's just making sure the bookkeeping doesn't become a burden.

[...] and would the impact of changes on existing implementers be high?

No concrete codebases of this lib yet, as far as I'm aware :) FWIW my mental model is to allow making major sweeping (breaking) changes during v0.x.y and then stick to the rigors of semantic versioning for major version >= 1. A hazard for those that wish to develop on top of the current codebase.

and adding 'Dynamic service modules' and 'Service management'

I think that's worthy of being a separate issue entirely. Depending on the particulars, the services, portions of framework, and indeed the Application interface/concept may need to be re-engineered from their current state. That and this issue indeed result in similar effects (results in changes to Application), but they are worthy of being separate issues due to the different underlying motivations.

aschrijver commented 3 years ago

Completely agree. Thanks for the elaboration.

cjslep commented 3 years ago

OK, I've done an iteration in 289217fd68aa2e5c9aec51f3c80a396b261f19bc

I think there is a possibility the NewIDPath function could be moved to the C2S-only interface, but I would need to double-check the internals of go-fed/activity to determine if that is indeed the case.

cjslep commented 3 years ago

Hey @cyisfor let me know if you have a chance to re-review the interfaces, feedback appreciated!

cjslep commented 3 years ago

Without further input, I'm going to bump this issue to the next milestone version, so that any further changes can be expected to be a part of that one. I'd like to take the current one into 0.1.0.