better lock flow

bot/bot.go

@@ -25,7 +25,7 @@ type bot struct {
 	defaultNamespace string
 	configurations   map[string]Configuration
 
-	cacheLocker      *sync.Mutex
+	globalLocker     *sync.Mutex
 	namespaceMutexes *cache.Cache
 	repoIssueMutexes *cache.Cache
 }
@@ -42,24 +42,32 @@ func (b *bot) getCurrentConfiguration(namespace string) (Configuration, error) {
 	return configuration, nil
 }
 
-func (b *bot) processRules(configuration Configuration, data EventData) *HandledEventResult {
-	id := fmt.Sprintf("%s/%s#%d", data.GetOwner(), data.GetRepo(), data.GetNumber())
-	util.Logger.Debug("acquire global lock during rules process")
-	b.cacheLocker.Lock()
-	locker, exists := b.repoIssueMutexes.Get(id)
+func (b *bot) processRules(
+	namespaceLock *sync.Mutex,
+	configuration Configuration,
+	partial EventData,
+	r *http.Request) *HandledEventResult {
+	id := fmt.Sprintf("%s/%s#%d", partial.GetOwner(), partial.GetRepo(), partial.GetNumber())
+	util.Logger.Debug("acquire namespace lock during rules process")
+	issueLocker, exists := b.repoIssueMutexes.Get(id)
 	if !exists {
-		locker = &sync.Mutex{}
-		b.repoIssueMutexes.Set(id, locker, cache.DefaultExpiration)
+		issueLocker = &sync.Mutex{}
+		b.repoIssueMutexes.Set(id, issueLocker, cache.DefaultExpiration)
 	}
 	util.Logger.Debug("acquire repo issue %s lock during rules process", id)
-	locker.(*sync.Mutex).Lock()
-	defer locker.(*sync.Mutex).Unlock()
-	util.Logger.Debug("release global lock during rules process")
-	b.cacheLocker.Unlock()
+	issueLocker.(*sync.Mutex).Lock()
+	defer issueLocker.(*sync.Mutex).Unlock()
+	util.Logger.Debug("release namespace lock during rules process")
+	namespaceLock.Unlock()
 	applied := make([]Rule, 0)
 	result := &HandledEventResult{
 		AppliedRules: []string{},
 	}
+	data, ok := completeBuildFromRequest(configuration.GetClientConfig(), r)
+	if !ok {
+		util.Logger.Debug("Skipping rule processing for %s (couldn't build complete data)", id)
+		return result
+	}
 	for _, rule := range configuration.GetRules() {
 		if rule.Accept(data) {
 			util.Logger.Debug("Accepting rule %s for '%s'", rule.Name(), data.GetTitle())
@@ -78,7 +86,7 @@ func (b *bot) processRules(configuration Configuration, data EventData) *Handled
 
 func (b *bot) HandleEvent(r *http.Request) *HandledEventResult {
 	namespace := r.URL.Query().Get("namespace")
-	b.cacheLocker.Lock()
+	b.globalLocker.Lock()
 	util.Logger.Debug("acquire global lock during namespace process")
 	locker, exists := b.namespaceMutexes.Get(namespace)
 	if !exists {
@@ -88,7 +96,7 @@ func (b *bot) HandleEvent(r *http.Request) *HandledEventResult {
 	util.Logger.Debug("acquire namespace %s lock", namespace)
 	locker.(*sync.Mutex).Lock()
 	util.Logger.Debug("release global lock during namespace process")
-	b.cacheLocker.Unlock()
+	b.globalLocker.Unlock()
 	workingConfiguration, err := b.getCurrentConfiguration(namespace)
 	if err != nil {
 		locker.(*sync.Mutex).Unlock()
@@ -100,14 +108,13 @@ func (b *bot) HandleEvent(r *http.Request) *HandledEventResult {
 		return &HandledEventResult{Message: "Skipping rules processing (could be not supported event type)"}
 	}
 	util.Logger.Debug("release namespace %s lock", namespace)
-	locker.(*sync.Mutex).Unlock()
-	return b.processRules(workingConfiguration, data)
+	return b.processRules(locker.(*sync.Mutex), workingConfiguration, data, r)
 }
 
 func New(configPaths ...string) (Bot, error) {
 	b := &bot{
 		configurations:   make(map[string]Configuration),
-		cacheLocker:      &sync.Mutex{},
+		globalLocker:     &sync.Mutex{},
 		namespaceMutexes: cache.New(time.Minute, 30*time.Second),
 		repoIssueMutexes: cache.New(time.Minute, 20*time.Second),
 	}

bot/bot_test.go

@@ -14,8 +14,8 @@ func (m *mockEventDataBuilder) BuildFromRequest(config ClientConfig, r *http.Req
 	return &mockConditionEventData{Labels: m.Labels}, true, nil
 }
 
-func (*mockEventDataBuilder) Build(config ClientConfig, json string) (EventData, error) {
-	panic("implement me")
+func (m *mockEventDataBuilder) PartialBuildFromRequest(config ClientConfig, r *http.Request) (EventData, bool, error) {
+	return &mockConditionEventData{Labels: m.Labels}, true, nil
 }
 
 func buildRequest(t *testing.T, url string) *http.Request {

bot/connector/github/builder.go

@@ -86,7 +86,7 @@ func (builder *eventDataBuilder) checkProcessState() bool {
 	return builder.data.state != "closed"
 }
 
-func (builder *eventDataBuilder) BuildFromRequest(config bot.ClientConfig, r *http.Request) (bot.EventData, bool, error) {
+func (builder *eventDataBuilder) PartialBuildFromRequest(config bot.ClientConfig, r *http.Request) (bot.EventData, bool, error) {
 	githubEvent := r.Header.Get("X-Github-Event")
 	if githubEvent == "ping" {
 		util.Logger.Message("Got GitHub 'ping' event")
@@ -114,15 +114,22 @@ func (builder *eventDataBuilder) BuildFromRequest(config bot.ClientConfig, r *ht
 	}
 	repo := pl.Repository.Name
 	owner := pl.Repository.Owner.Login
-	builder.client = newClient(config, owner, repo)
-	builder.data = &eventData{client: builder.client, owner: owner, repo: repo}
+	builder.data = &eventData{owner: owner, repo: repo}
 	builder.readFromJson(pl)
-	builder.readFromClient()
 	return builder.data, builder.checkProcessState(), nil
 }
 
-func (*eventDataBuilder) Build(config bot.ClientConfig, json string) (bot.EventData, error) {
-	panic("implement me")
+func (builder *eventDataBuilder) BuildFromRequest(config bot.ClientConfig, r *http.Request) (bot.EventData, bool, error) {
+	_, ok, err := builder.PartialBuildFromRequest(config, r)
+	if !ok || err != nil {
+		return nil, ok, err
+	}
+	repo := builder.data.repo
+	owner := builder.data.owner
+	builder.client = newClient(config, owner, repo)
+	builder.data.client = builder.client
+	builder.readFromClient()
+	return builder.data, builder.checkProcessState(), nil
 }
 
 func init() {

bot/data.go

@@ -36,8 +36,8 @@ type EventData interface {
 }
 
 type EventDataBuilder interface {
+	PartialBuildFromRequest(config ClientConfig, r *http.Request) (EventData, bool, error)
 	BuildFromRequest(config ClientConfig, r *http.Request) (EventData, bool, error)
-	Build(config ClientConfig, json string) (EventData, error)
 }
 
 var builders map[string]EventDataBuilder = make(map[string]EventDataBuilder)
@@ -54,6 +54,31 @@ func RegisterNewBuilder(provider string, builder EventDataBuilder) {
 }
 
 func buildFromRequest(config ClientConfig, r *http.Request) (EventData, bool) {
+	var builder EventDataBuilder
+	for name := range r.Header {
+		for provider := range builders {
+			if strings.Contains(strings.ToLower(name), provider) {
+				builder = builders[provider]
+				break
+			}
+		}
+		if builder != nil {
+			break
+		}
+	}
+	if builder == nil {
+		util.Logger.Error("No Builder to work with!")
+		return nil, false
+	}
+	result, process, err := builder.PartialBuildFromRequest(config, r)
+	if err != nil {
+		util.Logger.Error("Unable to build from request. %s", err)
+		return nil, false
+	}
+	return result, process
+}
+
+func completeBuildFromRequest(config ClientConfig, r *http.Request) (EventData, bool) {
 	var builder EventDataBuilder
 	for name := range r.Header {
 		for provider := range builders {