filtering: fix url edit

internal/filtering/filter.go

@@ -54,74 +54,77 @@ func (filter *FilterYAML) Path(dataDir string) string {
 }
 
 const (
-	statusFound = 1 << iota
-	statusEnabledChanged
-	statusURLChanged
-	statusURLExists
-	statusUpdateRequired
+	// errFilterNotExist is returned from [filterSetProperties] when there are
+	// no lists with the desired URL to update.
+	errFilterNotExist errors.Error = "url doesn't exist"
+
+	// errFilterExists is returned from [filterSetProperties] when there is
+	// another filter having the same URL as the one updated.
+	errFilterExists errors.Error = "url already exists"
 )
 
-// Update properties for a filter specified by its URL
-// Return status* flags.
-func (d *DNSFilter) filterSetProperties(url string, newf FilterYAML, whitelist bool) int {
-	r := 0
+// filterSetProperties searches for the particular filter list by url and sets
+// the values of newf to it, updating afterwards if needed.  It returns true if
+// the update was performed and the filtering engine is needed to be restarted.
+func (d *DNSFilter) filterSetProperties(
+	url string,
+	newf FilterYAML,
+	white bool,
+) (restart bool, err error) {
 	d.filtersMu.Lock()
 	defer d.filtersMu.Unlock()
 
 	filters := d.Filters
-	if whitelist {
+	if white {
 		filters = d.WhitelistFilters
 	}
 
-	i := slices.IndexFunc(filters, func(filt FilterYAML) bool {
-		return filt.URL == url
-	})
+	i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == url })
 	if i == -1 {
-		return 0
+		return false, errFilterNotExist
 	}
 
 	filt := &filters[i]
 
 	log.Debug("filter: set properties: %s: {%s %s %v}", filt.URL, newf.Name, newf.URL, newf.Enabled)
+
+	defer func(oldURL, oldName string, oldEnabled bool) {
+		if err != nil {
+			filt.URL = oldURL
+			filt.Name = oldName
+			filt.Enabled = oldEnabled
+		}
+	}(filt.URL, filt.Name, filt.Enabled)
+
 	filt.Name = newf.Name
 
 	if filt.URL != newf.URL {
-		r |= statusURLChanged | statusUpdateRequired
 		if d.filterExistsNoLock(newf.URL) {
-			return statusURLExists
+			return false, errFilterExists
 		}
 
+		restart = true
+
 		filt.URL = newf.URL
-		filt.unload()
 		filt.LastUpdated = time.Time{}
-		filt.checksum = 0
-		filt.RulesCount = 0
+		filt.unload()
 	}
 
 	if filt.Enabled != newf.Enabled {
-		r |= statusEnabledChanged
 		filt.Enabled = newf.Enabled
-		if filt.Enabled {
-			if (r & statusURLChanged) == 0 {
-				err := d.load(filt)
-				if err != nil {
-					// TODO(e.burkov):  It seems the error is only returned when
-					// the file exists and couldn't be open.  Investigate and
-					// improve.
-					log.Error("loading filter %d: %s", filt.ID, err)
-
-					filt.LastUpdated = time.Time{}
-					filt.checksum = 0
-					filt.RulesCount = 0
-					r |= statusUpdateRequired
-				}
-			}
-		} else {
-			filt.unload()
+		restart = true
+	}
+
+	if filt.Enabled {
+		if restart {
+			// Download the filter contents.
+			restart, err = d.update(filt)
 		}
+	} else {
+		filt.unload()
 	}
 
-	return r | statusFound
+	return restart, err
 }
 
 // Return TRUE if a filter with this URL exists
@@ -258,18 +261,6 @@ func (d *DNSFilter) tryRefreshFilters(block, allow, force bool) (updated int, is
 	return updated, isNetworkErr, ok
 }
 
-// refreshFilters updates the lists and returns the number of updated ones.
-// It's safe for concurrent use, but blocks at least until the previous
-// refreshing is finished.
-func (d *DNSFilter) refreshFilters(block, allow, force bool) (updated int) {
-	d.refreshLock.Lock()
-	defer d.refreshLock.Unlock()
-
-	updated, _ = d.refreshFiltersIntl(block, allow, force)
-
-	return updated
-}
-
 // listsToUpdate returns the slice of filter lists that could be updated.
 func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []FilterYAML) {
 	now := time.Now()
@@ -279,7 +270,6 @@ func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []Fi
 
 	for i := range *filters {
 		flt := &(*filters)[i] // otherwise we will be operating on a copy
-		log.Debug("checking list at index %d: %v", i, flt)
 
 		if !flt.Enabled {
 			continue

internal/filtering/http.go

@@ -56,7 +56,6 @@ func (d *DNSFilter) handleFilteringAddURL(w http.ResponseWriter, r *http.Request
 
 	err = validateFilterURL(fj.URL)
 	if err != nil {
-		err = fmt.Errorf("invalid url: %s", err)
 		aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)
 
 		return
@@ -75,8 +74,10 @@ func (d *DNSFilter) handleFilteringAddURL(w http.ResponseWriter, r *http.Request
 		URL:     fj.URL,
 		Name:    fj.Name,
 		white:   fj.Whitelist,
+		Filter: Filter{
+			ID: assignUniqueFilterID(),
+		},
 	}
-	filt.ID = assignUniqueFilterID()
 
 	// Download the filter contents
 	ok, err := d.update(&filt)
@@ -216,32 +217,15 @@ func (d *DNSFilter) handleFilteringSetURL(w http.ResponseWriter, r *http.Request
 		Name:    fj.Data.Name,
 		URL:     fj.Data.URL,
 	}
-	status := d.filterSetProperties(fj.URL, filt, fj.Whitelist)
-	if (status & statusFound) == 0 {
-		aghhttp.Error(r, w, http.StatusBadRequest, "URL doesn't exist")
 
-		return
-	}
-	if (status & statusURLExists) != 0 {
-		aghhttp.Error(r, w, http.StatusBadRequest, "URL already exists")
+	restart, err := d.filterSetProperties(fj.URL, filt, fj.Whitelist)
+	if err != nil {
+		aghhttp.Error(r, w, http.StatusBadRequest, err.Error())
 
 		return
 	}
 
 	d.ConfigModified()
-
-	restart := (status & statusEnabledChanged) != 0
-	if (status&statusUpdateRequired) != 0 && fj.Data.Enabled {
-		// download new filter and apply its rules.
-		nUpdated := d.refreshFilters(!fj.Whitelist, fj.Whitelist, false)
-		// if at least 1 filter has been updated, refreshFilters() restarts the filtering automatically
-		// if not - we restart the filtering ourselves
-		restart = false
-		if nUpdated == 0 {
-			restart = true
-		}
-	}
-
 	if restart {
 		d.EnableFilters(true)
 	}