refactor: comprehensive code quality improvements

- Add logger to all exporters for consistent error handling - Add Course.Validate() method with ValidationError type - Create shared test fixtures in internal/testdata - Refactor config passing: NewArticulateParser now takes *config.Config - Add NewLoggerFromConfig() factory for logger creation - Add context cancellation with signal handling (SIGINT/SIGTERM) - Add godoc examples for loggers and exporters - Add HTML exporter error path tests - Add fuzz tests for parser and HTMLCleaner - Fix markdown output: trim whitespace, collapse blank lines - Remove test output files from repo
2026-10-13 01:35:27 +02:00 · 2026-09-21 09:45:05 +02:00
parent 8ec9dd4f20
commit adb60ca7e5
29 changed files with 1940 additions and 415 deletions
@@ -31,33 +31,46 @@ func NewApp(parser interfaces.CourseParser, exporterFactory interfaces.ExporterF
 }

 // ProcessCourseFromFile loads a course from a local file and exports it to the specified format.
-// It takes the path to the course file, the desired export format, and the output file path.
-// Returns an error if loading or exporting fails.
-func (a *App) ProcessCourseFromFile(filePath, format, outputPath string) error {
+// It takes a context for cancellation, the path to the course file, the desired export format,
+// and the output file path. Returns an error if loading or exporting fails, or if the context
+// is canceled.
+func (a *App) ProcessCourseFromFile(ctx context.Context, filePath, format, outputPath string) error {
+	// Check for cancellation before loading
+	if err := ctx.Err(); err != nil {
+		return fmt.Errorf("operation canceled: %w", err)
+	}
+
 	course, err := a.parser.LoadCourseFromFile(filePath)
 	if err != nil {
 		return fmt.Errorf("failed to load course from file: %w", err)
 	}

-	return a.exportCourse(course, format, outputPath)
+	return a.exportCourse(ctx, course, format, outputPath)
 }

 // ProcessCourseFromURI fetches a course from the provided URI and exports it to the specified format.
-// It takes the URI to fetch the course from, the desired export format, and the output file path.
-// Returns an error if fetching or exporting fails.
+// It takes a context for cancellation, the URI to fetch the course from, the desired export format,
+// and the output file path. Returns an error if fetching or exporting fails, or if the context
+// is canceled.
 func (a *App) ProcessCourseFromURI(ctx context.Context, uri, format, outputPath string) error {
 	course, err := a.parser.FetchCourse(ctx, uri)
 	if err != nil {
 		return fmt.Errorf("failed to fetch course: %w", err)
 	}

-	return a.exportCourse(course, format, outputPath)
+	return a.exportCourse(ctx, course, format, outputPath)
 }

 // exportCourse exports a course to the specified format and output path.
 // It's a helper method that creates the appropriate exporter and performs the export.
-// Returns an error if creating the exporter or exporting the course fails.
-func (a *App) exportCourse(course *models.Course, format, outputPath string) error {
+// Returns an error if creating the exporter or exporting the course fails, or if the
+// context is canceled.
+func (a *App) exportCourse(ctx context.Context, course *models.Course, format, outputPath string) error {
+	// Check for cancellation before export
+	if err := ctx.Err(); err != nil {
+		return fmt.Errorf("operation canceled: %w", err)
+	}
+
 	exporter, err := a.exporterFactory.CreateExporter(format)
 	if err != nil {
 		return fmt.Errorf("failed to create exporter: %w", err)
@@ -207,7 +207,7 @@ func TestApp_ProcessCourseFromFile(t *testing.T) {
 			tt.setupMocks(parser, factory, exporter)

 			app := NewApp(parser, factory)
-			err := app.ProcessCourseFromFile(tt.filePath, tt.format, tt.outputPath)
+			err := app.ProcessCourseFromFile(context.Background(), tt.filePath, tt.format, tt.outputPath)

 			if tt.expectedError != "" {
 				if err == nil {
@@ -5,7 +5,10 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"log/slog"
+	"time"

+	"github.com/kjanat/articulate-parser/internal/config"
 	"github.com/kjanat/articulate-parser/internal/services"
 )

@@ -14,8 +17,8 @@ func ExampleNewArticulateParser() {
 	// Create a no-op logger for this example
 	logger := services.NewNoOpLogger()

-	// Create parser with defaults
-	parser := services.NewArticulateParser(logger, "", 0)
+	// Create parser with defaults (nil config uses defaults)
+	parser := services.NewArticulateParser(logger, nil)

 	fmt.Printf("Parser created: %T\n", parser)
 	// Output: Parser created: *services.ArticulateParser
@@ -26,11 +29,11 @@ func ExampleNewArticulateParser_custom() {
 	logger := services.NewNoOpLogger()

 	// Create parser with custom base URL and timeout
-	parser := services.NewArticulateParser(
-		logger,
-		"https://custom.articulate.com",
-		60_000_000_000, // 60 seconds in nanoseconds
-	)
+	cfg := &config.Config{
+		BaseURL:        "https://custom.articulate.com",
+		RequestTimeout: 60 * time.Second,
+	}
+	parser := services.NewArticulateParser(logger, cfg)

 	fmt.Printf("Parser configured: %T\n", parser)
 	// Output: Parser configured: *services.ArticulateParser
@@ -39,7 +42,7 @@ func ExampleNewArticulateParser_custom() {
 // ExampleArticulateParser_LoadCourseFromFile demonstrates loading a course from a file.
 func ExampleArticulateParser_LoadCourseFromFile() {
 	logger := services.NewNoOpLogger()
-	parser := services.NewArticulateParser(logger, "", 0)
+	parser := services.NewArticulateParser(logger, nil)

 	// In a real scenario, you'd have an actual file
 	// This example shows the API usage
@@ -52,7 +55,7 @@ func ExampleArticulateParser_LoadCourseFromFile() {
 // ExampleArticulateParser_FetchCourse demonstrates fetching a course from a URI.
 func ExampleArticulateParser_FetchCourse() {
 	logger := services.NewNoOpLogger()
-	parser := services.NewArticulateParser(logger, "", 0)
+	parser := services.NewArticulateParser(logger, nil)

 	// Create a context with timeout
 	ctx := context.Background()
@@ -64,6 +67,54 @@ func ExampleArticulateParser_FetchCourse() {
 	}
 }

+// ExampleNewSlogLogger demonstrates creating a JSON logger.
+func ExampleNewSlogLogger() {
+	// Create a JSON structured logger at Info level
+	// Output goes to stdout in JSON format
+	logger := services.NewSlogLogger(slog.LevelInfo)
+
+	// Logger is ready to use - calls like logger.Info("msg") output JSON
+	fmt.Printf("Logger type: %T\n", logger)
+	// Output: Logger type: *services.SlogLogger
+}
+
+// ExampleNewTextLogger demonstrates creating a human-readable text logger.
+func ExampleNewTextLogger() {
+	// Create a text logger, more readable for development
+	logger := services.NewTextLogger(slog.LevelDebug)
+
+	// Logger is ready - calls like logger.Debug("msg") output text
+	fmt.Printf("Logger type: %T\n", logger)
+	// Output: Logger type: *services.SlogLogger
+}
+
+// ExampleNewNoOpLogger demonstrates creating a no-op logger for testing.
+func ExampleNewNoOpLogger() {
+	logger := services.NewNoOpLogger()
+
+	// All log calls are silently discarded
+	logger.Debug("this won't appear")
+	logger.Info("neither will this")
+	logger.Error("or this")
+
+	fmt.Println("Logging silently completed")
+	// Output: Logging silently completed
+}
+
+// ExampleSlogLogger_With demonstrates creating a child logger with context.
+func ExampleSlogLogger_With() {
+	logger := services.NewNoOpLogger()
+
+	// Create a child logger with additional context
+	requestLogger := logger.With("request_id", "abc123", "user", "john")
+
+	// All logs from requestLogger will include request_id and user
+	requestLogger.Info("processing request")
+
+	fmt.Printf("Child logger type: %T\n", requestLogger)
+	// Output: Child logger type: *services.NoOpLogger
+}
+
 // ExampleHTMLCleaner demonstrates cleaning HTML content.
 func ExampleHTMLCleaner() {
 	cleaner := services.NewHTMLCleaner()
@@ -0,0 +1,15 @@
+package services
+
+import (
+	"github.com/kjanat/articulate-parser/internal/config"
+	"github.com/kjanat/articulate-parser/internal/interfaces"
+)
+
+// NewLoggerFromConfig creates a logger based on the configuration.
+// Returns a JSON logger if cfg.LogFormat is "json", otherwise returns a text logger.
+func NewLoggerFromConfig(cfg *config.Config) interfaces.Logger {
+	if cfg.LogFormat == "json" {
+		return NewSlogLogger(cfg.LogLevel)
+	}
+	return NewTextLogger(cfg.LogLevel)
+}
@@ -9,17 +9,14 @@ import (
 	"net/url"
 	"os"
 	"regexp"
-	"time"

+	"github.com/kjanat/articulate-parser/internal/config"
 	"github.com/kjanat/articulate-parser/internal/interfaces"
 	"github.com/kjanat/articulate-parser/internal/models"
 )

-// Default endpoint configuration for the Articulate Rise API.
-const (
-	riseHost       = "rise.articulate.com"
-	defaultBaseURL = "https://" + riseHost
-)
+// riseHost is the only host accepted in Articulate Rise share URIs.
+const riseHost = "rise.articulate.com"

 // shareIDRegex is compiled once at package init for extracting share IDs from URIs.
 var shareIDRegex = regexp.MustCompile(`/share/([a-zA-Z0-9_-]+)`)
@@ -35,23 +32,19 @@ type ArticulateParser struct {
 	Logger interfaces.Logger
 }

-// NewArticulateParser creates a new ArticulateParser instance.
-// If baseURL is empty, uses the default Articulate Rise API URL.
-// If timeout is zero, uses a 30-second timeout.
-func NewArticulateParser(logger interfaces.Logger, baseURL string, timeout time.Duration) interfaces.CourseParser {
+// NewArticulateParser creates a new ArticulateParser instance using the provided configuration.
+// If cfg is nil, a default configuration will be used.
+func NewArticulateParser(logger interfaces.Logger, cfg *config.Config) interfaces.CourseParser {
 	if logger == nil {
 		logger = NewNoOpLogger()
 	}
-	if baseURL == "" {
-		baseURL = defaultBaseURL
-	}
-	if timeout == 0 {
-		timeout = 30 * time.Second
+	if cfg == nil {
+		cfg = config.Load()
 	}
 	return &ArticulateParser{
-		BaseURL: baseURL,
+		BaseURL: cfg.BaseURL,
 		Client: &http.Client{
-			Timeout: timeout,
+			Timeout: cfg.RequestTimeout,
 		},
 		Logger: logger,
 	}
@@ -8,7 +8,9 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"

+	"github.com/kjanat/articulate-parser/internal/config"
 	"github.com/kjanat/articulate-parser/internal/models"
 )

@@ -133,7 +135,11 @@ func BenchmarkArticulateParser_LoadCourseFromFile(b *testing.B) {
 		b.Fatalf("Failed to write file: %v", err)
 	}

-	parser := NewArticulateParser(nil, "", 0)
+	cfg := &config.Config{
+		BaseURL:        "https://rise.articulate.com",
+		RequestTimeout: 30 * time.Second,
+	}
+	parser := NewArticulateParser(nil, cfg)

 	b.ResetTimer()
 	for b.Loop() {
@@ -180,7 +186,11 @@ func BenchmarkArticulateParser_LoadCourseFromFile_Large(b *testing.B) {
 		b.Fatalf("Failed to write file: %v", err)
 	}

-	parser := NewArticulateParser(nil, "", 0)
+	cfg := &config.Config{
+		BaseURL:        "https://rise.articulate.com",
+		RequestTimeout: 30 * time.Second,
+	}
+	parser := NewArticulateParser(nil, cfg)

 	b.ResetTimer()
 	for b.Loop() {
@@ -0,0 +1,207 @@
+package services
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/kjanat/articulate-parser/internal/models"
+)
+
+// FuzzArticulateParser_ParseJSON fuzzes JSON parsing to ensure no panics on malformed input.
+// It tests that the parser gracefully handles any byte sequence without crashing.
+func FuzzArticulateParser_ParseJSON(f *testing.F) {
+	// Add seed corpus with valid JSON structures
+	f.Add([]byte(`{"shareId":"test","course":{"id":"1","title":"Test"}}`))
+	f.Add([]byte(`{"shareId":"abc123","author":"John Doe","course":{"id":"c1","title":"Course Title","description":"A test course"}}`))
+	f.Add([]byte(`{}`))
+	f.Add([]byte(`{"course":{}}`))
+	f.Add([]byte(`{"shareId":"","course":{"id":"","title":"","lessons":[]}}`))
+
+	// Add seed corpus with invalid/edge case inputs
+	f.Add([]byte(`invalid`))
+	f.Add([]byte(``))
+	f.Add([]byte(`null`))
+	f.Add([]byte(`[]`))
+	f.Add([]byte(`"string"`))
+	f.Add([]byte(`123`))
+	f.Add([]byte(`true`))
+	f.Add([]byte(`{`))
+	f.Add([]byte(`}`))
+	f.Add([]byte(`{"unclosed": `))
+	f.Add([]byte(`{"key": "value with \x00 null byte"}`))
+
+	// Add seed corpus with nested structures
+	f.Add([]byte(`{"course":{"lessons":[{"id":"l1","title":"Lesson 1","items":[{"id":"i1","type":"text","data":"content"}]}]}}`))
+	f.Add([]byte(`{"course":{"lessons":[{},{},{}]}}`))
+
+	// Add seed corpus with special characters
+	f.Add([]byte(`{"shareId":"test<script>alert('xss')</script>"}`))
+	f.Add([]byte(`{"course":{"title":"Test\n\r\t\u0000Title"}}`))
+	f.Add([]byte(`{"course":{"description":"<p>HTML content</p>"}}`))
+
+	f.Fuzz(func(t *testing.T, data []byte) {
+		// Test that unmarshaling doesn't panic
+		var course models.Course
+		_ = json.Unmarshal(data, &course)
+
+		// Also test that re-marshaling doesn't panic
+		_, _ = json.Marshal(&course)
+
+		// Test accessing fields doesn't panic
+		_ = course.ShareID
+		_ = course.Author
+		_ = course.Course.ID
+		_ = course.Course.Title
+		_ = course.Course.Description
+
+		// Test iterating over lessons doesn't panic
+		for _, lesson := range course.Course.Lessons {
+			_ = lesson.ID
+			_ = lesson.Title
+			for _, item := range lesson.Items {
+				_ = item.ID
+				_ = item.Type
+			}
+		}
+	})
+}
+
+// FuzzArticulateParser_LoadCourseFromFile_JSON tests JSON parsing behavior used by LoadCourseFromFile.
+// This fuzz test ensures the JSON unmarshaling logic handles arbitrary input gracefully.
+func FuzzArticulateParser_LoadCourseFromFile_JSON(f *testing.F) {
+	// Seed corpus mimicking file contents
+	f.Add([]byte(`{"shareId":"file-test","course":{"id":"1","title":"From File"}}`))
+	f.Add([]byte(`{
+		"shareId": "multiline",
+		"course": {
+			"id": "2",
+			"title": "Multiline JSON"
+		}
+	}`))
+	f.Add([]byte("\xef\xbb\xbf{\"shareId\":\"bom\"}")) // UTF-8 BOM prefix
+	f.Add([]byte(`{"shareId": "spaces",  "course":  {}}`))
+
+	f.Fuzz(func(t *testing.T, data []byte) {
+		var course models.Course
+		err := json.Unmarshal(data, &course)
+
+		// If unmarshaling succeeded, verify the result is usable
+		if err == nil {
+			// Access all fields to ensure no panics on zero values
+			_ = course.ShareID
+			_ = course.Author
+			_ = course.Course.ID
+			_ = course.Course.Title
+
+			// Iterate through nested structures
+			for _, lesson := range course.Course.Lessons {
+				_ = lesson.ID
+				_ = lesson.Title
+				_ = lesson.Type
+				for _, item := range lesson.Items {
+					_ = item.ID
+					_ = item.Type
+					_ = item.Data
+				}
+			}
+		}
+	})
+}
+
+// FuzzHTMLCleaner_CleanHTML fuzzes the HTML cleaner to ensure no panics on malformed HTML.
+// It tests that arbitrary HTML-like input is handled gracefully.
+func FuzzHTMLCleaner_CleanHTML(f *testing.F) {
+	cleaner := NewHTMLCleaner()
+
+	// Add seed corpus with valid HTML patterns
+	f.Add("<p>Simple paragraph</p>")
+	f.Add("<div><p>Nested</p></div>")
+	f.Add("<h1>Title</h1><p>Content</p>")
+	f.Add("<strong>Bold</strong> and <em>italic</em>")
+	f.Add("<a href=\"https://example.com\">Link</a>")
+	f.Add("<img src=\"image.png\" alt=\"Image\"/>")
+	f.Add("<br/><hr/>")
+	f.Add("<ul><li>Item 1</li><li>Item 2</li></ul>")
+	f.Add("<table><tr><td>Cell</td></tr></table>")
+
+	// Add seed corpus with HTML entities
+	f.Add("AT&amp;T")
+	f.Add("&lt;script&gt;alert('xss')&lt;/script&gt;")
+	f.Add("&nbsp;&nbsp;&nbsp;")
+	f.Add("&#39;apostrophe&#39;")
+	f.Add("&quot;quoted&quot;")
+	f.Add("&eacute;&iuml;&ouml;")
+	f.Add("&#x27;hex entity&#x27;")
+	f.Add("&#8217;numeric entity&#8217;")
+
+	// Add seed corpus with malformed/edge case HTML
+	f.Add("")
+	f.Add("plain text no HTML")
+	f.Add("<p>Unclosed paragraph")
+	f.Add("</p>Close tag first<p>")
+	f.Add("<div><div><div>Deep nesting")
+	f.Add("<script>alert('xss')</script>")
+	f.Add("<style>body{color:red}</style>")
+	f.Add("<!--comment-->")
+	f.Add("<!DOCTYPE html>")
+	f.Add("<![CDATA[data]]>")
+	f.Add("<p class=\"test\" id='test' data-attr=unquoted>Attributes</p>")
+	f.Add("<p\n\t>Whitespace in tag</p\n>")
+	f.Add("<>empty tag</>")
+	f.Add("<<<nested angles>>>")
+	f.Add("<p onclick=\"evil()\">Event handler</p>")
+
+	// Add seed corpus with special characters
+	f.Add("<p>\x00null byte</p>")
+	f.Add("<p>\xffhigh byte</p>")
+	f.Add("<p>emoji: \xf0\x9f\x98\x80</p>")
+	f.Add("<p>unicode: \u2603</p>")
+	f.Add("<p>newlines\n\r\nand\rtabs\there</p>")
+
+	// Add seed corpus with potentially problematic patterns
+	f.Add(string(make([]byte, 0)))                     // Empty
+	f.Add(string(make([]byte, 1)))                     // Single null byte
+	f.Add("<" + string(make([]byte, 100)) + ">")       // Tag with null bytes
+	f.Add("<p>" + string(make([]byte, 1000)) + "</p>") // Large content
+
+	f.Fuzz(func(t *testing.T, input string) {
+		// Test that CleanHTML doesn't panic on any input
+		result := cleaner.CleanHTML(input)
+
+		// Verify the result is a valid string (doesn't panic on access)
+		_ = len(result)
+
+		// Test that the result can be used in common string operations without panic
+		_ = result == ""
+		_ = result == input
+
+		// Test that calling CleanHTML on its own output doesn't panic (idempotency check)
+		_ = cleaner.CleanHTML(result)
+	})
+}
+
+// FuzzHTMLCleaner_CleanHTML_Bytes tests HTML cleaner with raw byte sequences.
+// This catches issues with invalid UTF-8 and binary data in HTML content.
+func FuzzHTMLCleaner_CleanHTML_Bytes(f *testing.F) {
+	cleaner := NewHTMLCleaner()
+
+	// Add seed corpus with byte sequences
+	f.Add([]byte("<p>normal</p>"))
+	f.Add([]byte{0x3c, 0x70, 0x3e, 0xff, 0xfe, 0x3c, 0x2f, 0x70, 0x3e})       // <p>..invalid..</p>
+	f.Add([]byte{0x00})                                                       // Null byte
+	f.Add([]byte{0xff, 0xfe})                                                 // Invalid UTF-8
+	f.Add([]byte{0xef, 0xbb, 0xbf, 0x3c, 0x70, 0x3e, 0x3c, 0x2f, 0x70, 0x3e}) // UTF-8 BOM + <p></p>
+	f.Add([]byte("<p>\xc0\xaf</p>"))                                          // Overlong UTF-8
+
+	f.Fuzz(func(t *testing.T, data []byte) {
+		// Convert bytes to string and test
+		input := string(data)
+
+		// Test that CleanHTML doesn't panic
+		result := cleaner.CleanHTML(input)
+
+		// Verify result is usable
+		_ = len(result)
+		_ = []byte(result)
+	})
+}
@@ -11,12 +11,14 @@ import (
 	"testing"
 	"time"

+	"github.com/kjanat/articulate-parser/internal/config"
 	"github.com/kjanat/articulate-parser/internal/models"
 )

 // TestNewArticulateParser tests the NewArticulateParser constructor.
 func TestNewArticulateParser(t *testing.T) {
-	parser := NewArticulateParser(nil, "", 0)
+	// Test with nil config (should use defaults)
+	parser := NewArticulateParser(nil, nil)

 	if parser == nil {
 		t.Fatal("NewArticulateParser() returned nil")
@@ -212,7 +214,11 @@ func TestArticulateParser_LoadCourseFromFile(t *testing.T) {
 		t.Fatalf("Failed to write test file: %v", err)
 	}

-	parser := NewArticulateParser(nil, "", 0)
+	cfg := &config.Config{
+		BaseURL:        "https://rise.articulate.com",
+		RequestTimeout: 30 * time.Second,
+	}
+	parser := NewArticulateParser(nil, cfg)

 	tests := []struct {
 		name          string
@@ -271,7 +277,11 @@ func TestArticulateParser_LoadCourseFromFile_InvalidJSON(t *testing.T) {
 		t.Fatalf("Failed to write test file: %v", err)
 	}

-	parser := NewArticulateParser(nil, "", 0)
+	cfg := &config.Config{
+		BaseURL:        "https://rise.articulate.com",
+		RequestTimeout: 30 * time.Second,
+	}
+	parser := NewArticulateParser(nil, cfg)
 	_, err := parser.LoadCourseFromFile(tempFile)

 	if err == nil {