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

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

Open
cyisfor opened this issue Nov 29, 2020 · 11 comments
Labels
feature request New request for novel functionality

Comments

@cyisfor
Copy link

cyisfor commented Nov 29, 2020

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
Copy link
Author

cyisfor commented Nov 29, 2020

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 cyisfor closed this as completed Nov 29, 2020
@cyisfor
Copy link
Author

cyisfor commented Nov 29, 2020

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
Copy link
Member

cjslep commented Nov 29, 2020

(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:

  • Someone wants to have an application that federates S2S, but have a traditional REST API (and, therefore, custom clients that are not really interoperable with other software).
  • Someone that doesn't really want federation and favors local community building, but wants a C2S API, possibly in addition to a REST API (to leverage the diversity of clients that support the C side of C2S, which is admittedly more theoretical at this point).
  • Someone that wants both.

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 cjslep reopened this Nov 29, 2020
@cjslep
Copy link
Member

cjslep commented Nov 29, 2020

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
Copy link
Member

cjslep commented Nov 29, 2020

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.

@cjslep cjslep changed the title C2S/S2S are not embedded interfaces C2S/S2S are not embedded interfaces (iterate on Application & related interfaces) Nov 29, 2020
@cjslep cjslep added this to the v1.0.0 milestone Nov 29, 2020
@cjslep cjslep added the feature request New request for novel functionality label Nov 29, 2020
@aschrijver
Copy link

aschrijver commented Nov 29, 2020

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 cjslep self-assigned this Dec 8, 2020
@cjslep
Copy link
Member

cjslep commented Dec 8, 2020

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
Copy link

Completely agree. Thanks for the elaboration.

@cjslep
Copy link
Member

cjslep commented Dec 13, 2020

OK, I've done an iteration in 289217f

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
Copy link
Member

cjslep commented Dec 14, 2020

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

@cjslep cjslep removed their assignment Dec 16, 2020
@cjslep
Copy link
Member

cjslep commented Dec 19, 2020

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.

@cjslep cjslep removed this from the v0.1.0 milestone Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New request for novel functionality
Projects
None yet
Development

No branches or pull requests

3 participants